> On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > Initial pass, curious if the other changes bundled in this review could be > > pulled out.
I need to kill get() and getAll() in this review due to the new semantics (also because those two interfaces are very poorly designed). find() uses get() and getAll(). > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 125-130 > > <https://reviews.apache.org/r/28091/diff/2/?file=765011#file765011line125> > > > > Why is find() changed in this review? Is it related to flattening in > > some way? See my above comments. > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 46-48 > > <https://reviews.apache.org/r/28091/diff/2/?file=765011#file765011line46> > > > > I'm curious, where is this relevant to the user of 'Resources'? Now > > that this TODO is implemented, does the user of 'Resources' need this NOTE? > > > > Maybe the important NOTE is that the caller *needs* to ensure that any > > externally specified resources are validated, right? Add notes about validation. I still keep the existing notes because I think as a reader of this code, he/she needs to know that. > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 75-76 > > <https://reviews.apache.org/r/28091/diff/2/?file=765011#file765011line75> > > > > Since you're changing the name of `isZero` to `zero`, how about > > `empty()` to be symmetric with resources.empty()? Done. > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 132-135 > > <https://reviews.apache.org/r/28091/diff/2/?file=765011#file765011line132> > > > > I'm a bit confused at the utility of these, do you need to expose them > > in this change? > > > > Also, why return `Value::*`? Why not return C++ types, like double, > > std::set, and IntervalSet? This is due to a cascading effect. Many tests use the get() interface which I removed in this review. It's actually the template<> get() function in the old version. I just renamed it to get rid of the template: old: get("ports", Value::Ranges()); new: getRanges("ports"); The function do not return double, set or IntervalSet because many of the tests expect Value::Set/Value::Ranges in the tests. We can iterate > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 387 > > <https://reviews.apache.org/r/28091/diff/2/?file=765012#file765012line387> > > > > Why does this need to be prefixed with _? Fixed. > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > src/tests/gc_tests.cpp, lines 279-280 > > <https://reviews.apache.org/r/28091/diff/2/?file=765021#file765021line279> > > > > Looks like these tests should just be using resources.cpus() and > > resources.mem()? This is symetric to Set and Ranges tests. If you insist, we can fix that in a followup patch > On Nov. 15, 2014, 7:53 p.m., Ben Mahler wrote: > > src/tests/resource_offers_tests.cpp, lines 656-657 > > <https://reviews.apache.org/r/28091/diff/2/?file=765023#file765023line656> > > > > Ditto here and everywhere else w.r.t. cpus() and mem(). Ditto my above comments - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28091/#review61656 ----------------------------------------------------------- On Nov. 15, 2014, 7:18 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28091/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2014, 7:18 a.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-1974 > https://issues.apache.org/jira/browse/MESOS-1974 > > > Repository: mesos-git > > > Description > ------- > > See summary. Always combine Resource objects in Resources and disallow > invalid/zero Resource objects. > > > Diffs > ----- > > include/mesos/resources.hpp 0e37170 > src/common/resources.cpp e9a0c85 > src/examples/low_level_scheduler_libprocess.cpp 89b4318 > src/examples/low_level_scheduler_pthread.cpp e5cd48a > src/examples/test_framework.cpp e87198b > src/master/drf_sorter.cpp 5464900 > src/master/hierarchical_allocator_process.hpp 31dfb2c > src/master/http.cpp 3189933 > src/master/master.cpp 83c2f8a > src/slave/containerizer/containerizer.cpp f234835 > src/tests/gc_tests.cpp 8618ae1 > src/tests/mesos.hpp c1d64a7 > src/tests/resource_offers_tests.cpp 43820b0 > src/tests/resources_tests.cpp 3e50889 > > Diff: https://reviews.apache.org/r/28091/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
