> On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote: > > include/mesos/resources.hpp > > Lines 45 (patched) > > <https://reviews.apache.org/r/69601/diff/2/?file=2116929#file2116929line45> > > > > This is a public header and I don't think the internal headers get > > installed so this would break any use of the public headers? > > > > Can you forward declare instead?
Done. > On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote: > > src/common/resources.cpp > > Lines 1517 (patched) > > <https://reviews.apache.org/r/69601/diff/2/?file=2116931#file2116931line1517> > > > > Do we need the iterator? Can we use a foreach auto loop here? Done. > On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote: > > src/common/resources.cpp > > Lines 1523-1525 (patched) > > <https://reviews.apache.org/r/69601/diff/2/?file=2116931#file2116931line1523> > > > > Maybe we can make this a bit more tidy? > > > > ``` > > case Value::SCALAR: remaining -= r.scalar().value(); break; > > case Value::SET: remaining -= r.set().item_size(); break; > > case ... > > ``` Done. > On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote: > > src/common/resources.cpp > > Lines 1529 (patched) > > <https://reviews.apache.org/r/69601/diff/2/?file=2116931#file2116931line1529> > > > > Why the comment here but not below on the same optimization? > > > > It seems fine without the comment in both places to me Done. > On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote: > > src/tests/resources_tests.cpp > > Lines 2054 (patched) > > <https://reviews.apache.org/r/69601/diff/2/?file=2116932#file2116932line2054> > > > > Maybe we can make the tests more readable with some better variable > > names or lambdas that allow us to see the entire comparision on one line? > > > > E.g. > > > > ``` > > EXPECT_TRUE(Resources().contains(ResourceQuantities()); > > > > EXPECT_FALSE(nonEmptyResources.contains(emptyQuantities); > > > > auto resources = [](const string& s) { ... }; > > auto quantities = [](const string& s) { ... }; > > > > // Can easily read what's being checked: > > EXPECT_TRUE(resources("cpus:1;mem:2") > > .contains(quantities("cpus:0.1;mem:1")); > > ``` Done, much cleaner, thanks! - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69601/#review211638 ----------------------------------------------------------- On Jan. 4, 2019, 12:16 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69601/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2019, 12:16 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This method checks if the quantities of this `Resources` is a > superset of the given `ResourceQuantities`. If a `Resource` object > is `SCALAR` type, its quantity is its scalar value. For `RANGES` > and `SET` type, their quantities are the number of different > instances in the range or set. For example, "range:[1-5]" has a > quantity of 5 and "set:{a,b}" has a quantity of 2. > > Also added a dedicated test. > > > Diffs > ----- > > include/mesos/resources.hpp 36ccf0e1135ce15898d31db51c80fa7b5826907b > include/mesos/v1/resources.hpp 1a9ea44e78d985286e04e040879d022838ddcc3f > src/common/resources.cpp 758b5a2fac101bfb0a07ba76c204b02d1554b3ac > src/tests/resources_tests.cpp 5337a5c9af9431cc1c7822c78945ffc369488900 > src/v1/resources.cpp d717f5327a45cfe9bc075bc3acf37316223d389d > > > Diff: https://reviews.apache.org/r/69601/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
