> On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 191-202 > > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line191> > > > > Great, do we want to specify what a valid transformation is? In > > particular, it seems like the invariant of a Transformation is that the > > amount of a resource ("cpus", "mem", "disk", "ports", ...) remains > > unchanged and only the "metadata" of the resources can be manipulated. > > Let's document that this is what a Transformation is? > > > > For invariants, maybe just a NOTE for now.. but I'd love to have > > invariants enforced in the allocator (if not provided by Transformation the > > allocator will have to manually check them). > > > > One suggestion is, similary to the Registrar's Operation, we can keep a > > top level `apply` or function operator as non-virtual, which performs the > > transformation and validates any of the invariants (cpus, mem, disk, ports > > remain the same) and rely on a virtual method to perform the actual > > transformation? That way, Transformation can enforce the invariants, and a > > CHECK fails or an Error results if they are broken.
Added a non virtual operator which calls the virtual apply. Enforced the invariants in the operator. > On Dec. 15, 2014, 9:29 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 211-214 > > <https://reviews.apache.org/r/29018/diff/1/?file=791218#file791218line211> > > > > To keep consistent with the rest of the code base, couldn't you have > > done the following? > > > > ``` > > template <typename T> > > void add(const T& t) > > { > > transformations.push_back(new T(t)); > > } > > ``` > > > > With unique_ptr, we're requiring that ownership be transferred if the > > caller is holding the transformation, which seems a bit odd for our library > > to impose on the caller: > > > > ``` > > // The caller cannot keep ownership of t. > > std::unique_ptr<Transformation> t(new DiskTransformation(...)); > > composite.add(std::move(t)); > > > > // Ok. > > composite.add(std::unique_ptr<Transformation>(new > > DiskTransformation(...)); > > ``` > > > > Could we keep the const argument semantics for now for consistency? > > Then the caller doesn't have to use 'new' as well. Thoughts? > > Dominic Hamon wrote: > That's exactly the point of std::unique_ptr - a library is finally able > to inform the caller that they're taking responsibility for the object and > the memory associated with it. I don't see that as odd. > > Can you describe a use case where a caller might want to retain a > reference to the transformation? OK, since we haven't finalized the unique_ptr usage in the style guide, I'll use your suggestion and add a TODO here. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29018/#review65121 ----------------------------------------------------------- On Dec. 13, 2014, 5:57 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29018/ > ----------------------------------------------------------- > > (Updated Dec. 13, 2014, 5:57 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2030 > https://issues.apache.org/jira/browse/MESOS-2030 > > > Repository: mesos-git > > > Description > ------- > > Added abstraction for resources transformation. Split from > https://reviews.apache.org/r/28720/. > > > Diffs > ----- > > include/mesos/resources.hpp 296553af93ec8457dad04ac018f03f36df654fdc > src/common/resources.cpp 9bf7ae9148d6db2829cc2866ac048fe018ae2c92 > > Diff: https://reviews.apache.org/r/29018/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
