> On Feb. 15, 2014, 12:56 a.m., Dominic Hamon wrote: > >
FYI, we prefer to respond to review comments on the "main" page (i.e., the only one where you can read this comment). That way we can more easily track the thread across diffs. > On Feb. 15, 2014, 12:56 a.m., Dominic Hamon wrote: > > src/master/http.cpp, line 78 > > <https://reviews.apache.org/r/17976/diff/2/?file=486048#file486048line78> > > > > You're right, sorry. > > > > I do feel that it is much less readable (and maintainable) without the > > non-const reference. I'm still curious to understand why this perfectly > > valid use-case for references would be avoided in this case. I'm not convinced it's more readable or maintainable with the non-const reference. A reader needs to remember as they go through the code that 'role' is in fact a _pointer_ (masquerading as a reference) into the 'roles' map, so manipulating it is manipulating an object "someplace else". We avoid this style generally in Mesos (and libprocess, and stout), and while this piece of code is small it's better to be consistent everywhere. The statement 'roles[resource.role()].values[resource.name()] = resource.scalar().value();' is "complete" in the sense that in isolation you can understand exactly what is happening because it has no dependencies (or at least, less dependencies, i.e., you need to remember less to understand it). In my experience, less dependencies has typically yielded more maintainable code (because there is less state around that needs to be referenced, copied, etc., when doing any refactoring). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17976/#review34562 ----------------------------------------------------------- On Feb. 15, 2014, 12:56 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17976/ > ----------------------------------------------------------- > > (Updated Feb. 15, 2014, 12:56 a.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 > >
