----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29128/#review65262 -----------------------------------------------------------
Thanks for splitting! Can you simplify the Transformation per your suggestion when we chatted? (Simple transformation that is non-idempotent, master contains the validation around persistence IDs). Some of my comments will become invalid or will move to the master code. Then we can get this shipped, I'll need to update the sorters to not aggregate resources across slaves (or at least drop a TODO). I'll make a note on my review. include/mesos/resources.hpp <https://reviews.apache.org/r/29128/#comment108334> It might not be clear what "no transformation will be applied" means, error? How about: s/no transformation will be applied/no change occurs/ src/common/resources.cpp <https://reviews.apache.org/r/29128/#comment108337> How about: ``` if (resources.contains(disk)) { return resources; // No-op: already acquired! } src/common/resources.cpp <https://reviews.apache.org/r/29128/#comment108336> Is Resources() here necessary? It looks like there is an operator for just doing: ``` if (resources.contains(disk)) { ... } ``` Via this: ``` bool Resources::contains(const Resource& that) const; ``` src/common/resources.cpp <https://reviews.apache.org/r/29128/#comment108338> Might want to wrap the ID in single quotes, unless we're heavily sanitizing it (e.g. do we allow whitespace?) src/tests/resources_tests.cpp <https://reviews.apache.org/r/29128/#comment108344> Just curious, where will we catch a duplicate container path under the same executor? Do you have an integration test for this at least? Let's keep track of this extra case. - Ben Mahler On Dec. 16, 2014, 11:56 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29128/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2014, 11:56 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2030 > https://issues.apache.org/jira/browse/MESOS-2030 > > > Repository: mesos-git > > > Description > ------- > > See summary. Seprated out from https://reviews.apache.org/r/28720/ > > > Diffs > ----- > > include/mesos/resources.hpp 296553a > src/common/resources.cpp 9bf7ae9 > src/tests/resources_tests.cpp bac092e > > Diff: https://reviews.apache.org/r/29128/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
