----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29018/#review65121 -----------------------------------------------------------
Thanks Jie! include/mesos/resources.hpp <https://reviews.apache.org/r/29018/#comment108078> 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. include/mesos/resources.hpp <https://reviews.apache.org/r/29018/#comment108076> 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? - Ben Mahler 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 > >
