----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17976/#review34548 -----------------------------------------------------------
This is looking good, mostly just some stylistic nits below. src/master/http.cpp <https://reviews.apache.org/r/17976/#comment64667> Since there are already 'using' clauses above, we can remove the 'std::' qualifiers on map and string. How about: s/roleObjects/roles/ src/master/http.cpp <https://reviews.apache.org/r/17976/#comment64678> In general we avoid using non-const references to mutate data, what if we stored the role string: foreach(...) { const string& role = resource.role(); switch(...) { ... case Value::RANGES: roles[role].values[resource.name()] = stringify(resource.ranges()); break; ... } src/master/http.cpp <https://reviews.apache.org/r/17976/#comment64676> Can you add a newline here? src/master/http.cpp <https://reviews.apache.org/r/17976/#comment64669> Can you remove the 'std::' qualifier? Maybe s/roleObject/object/ or s/roleObject/resources/ ? src/webui/master/static/js/controllers.js <https://reviews.apache.org/r/17976/#comment64675> What is r? ;) - Ben Mahler On Feb. 14, 2014, 8:41 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17976/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2014, 8:41 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-692 > https://issues.apache.org/jira/browse/MESOS-692 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > old format of master/state.json: > {... > "slaves":[... > "resources":{"ports":"[31000-32000]"}} > ...] > ...} > > new format: > {... > "slaves":[... > "resources":{"*":{"ports":"[31000-32000]"}}} > ...] > ...} > > There is an assumption that there is each resource has a unique role. If this > is not the case (ie, if there are multiple resources with the role '*' a > warning is emitted. The solution for this is to correctly handle merging of > resources with the same role. > > > Diffs > ----- > > src/master/http.cpp 966eed6d8340038265ef799f1b6149502ccc606e > src/slave/http.cpp c4f598faf6807214608cc89a6d9cf665133f95f3 > src/webui/master/static/js/controllers.js > afb24fb9c2184772f7314162f5637dbabaa2ab94 > > Diff: https://reviews.apache.org/r/17976/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
