----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60013/#review177695 -----------------------------------------------------------
src/common/resources.cpp Lines 847-848 (patched) <https://reviews.apache.org/r/60013/#comment251415> We try not to use backticks in error or logging messages, they were introduced to provide markup for comments. Can you replace them with single quotes here and elsewhere? src/common/resources.cpp Lines 854 (patched) <https://reviews.apache.org/r/60013/#comment251416> Whoops, no periods in error messages src/common/resources.cpp Lines 844-845 (original), 864-868 (patched) <https://reviews.apache.org/r/60013/#comment251476> Do you want to add a function to roles.hpp for checking this? ``` bool isSubRole(const string& parentRole, const string& subRole); ``` src/common/resources.cpp Lines 846-849 (original), 870-914 (patched) <https://reviews.apache.org/r/60013/#comment251421> This was a little tough to read through, I wonder if the following would be easier to read: ``` // Validate all of the roles. foreach (const ReservationInfo& reservation, resource.reservations()) { Option<Error> error = roles::validate(reservation.role()); if (error.isSome()) { return error; } if (reservation.role() == "*") { return Error("Invalid reservation: role \"*\" cannot be reserved"); } } // Validate that each reservation is a refinement of the // preceding reservation. string parentRole = resource.reservations(0).role(); for (int i = 1; i < resource.reservations_size(); ++i) { if (resource.reservations(i).type() == Resource::ReservationInfo::STATIC) { return Error( "Invalid refined reservation: A refined reservation" " cannot be static."); } const string& childRole = resource.reservations(i).role(); if (!isSubRole(parentRole, childRole)) { return Error( "Invalid refined reservation: role '" + childRole + "' is not a refinement of '" + parentRole + "'"); } parentRole = role; } ``` Granted, this doesn't have your tokenize optimization, but perhaps for now we just add a TODO to optimize this? It seems to me there are bigger optimizations than just halving the number of tokenizations, like avoiding the copying that tokenization incurs altogether. src/common/resources.cpp Lines 907-908 (patched) <https://reviews.apache.org/r/60013/#comment251417> We try to open and close the quote on the same line (otherwise we tend to forget to close because it's not as obvious), can you do that here? src/master/validation.hpp Lines 139 (patched) <https://reviews.apache.org/r/60013/#comment251406> Option bool seems a little confusing to me, since I assume false is equivalent to None()? Why not just take optional framework info or to be more specific, optional framework capabilities here? src/master/validation.hpp Line 154 (original), 155 (patched) <https://reviews.apache.org/r/60013/#comment251407> Why not take optional framework capabilities or framework info here? Rather than the framework state? src/master/validation.hpp Line 182 (original), 183 (patched) <https://reviews.apache.org/r/60013/#comment251408> Ditto here src/master/validation.hpp Lines 188 (patched) <https://reviews.apache.org/r/60013/#comment251409> Ditto here. src/master/validation.cpp Lines 601-603 (patched) <https://reviews.apache.org/r/60013/#comment251413> Can you add a comment about what this is doing? i.e. that we have an old and new format for resource role / reservations? Perhaps name this `validateReservationFormat` or something that is a little more specific towards that? src/master/validation.cpp Lines 603 (patched) <https://reviews.apache.org/r/60013/#comment251412> Why not just take the capabilities here, instead of taking the bool? src/master/validation.cpp Lines 609-610 (patched) <https://reviews.apache.org/r/60013/#comment251410> I don't think we're using periods in these messages (we generally don't) Also, no backticks in error or logging messages (just comments) (some leaked in) Also, we have been generally trying to put the space on the next line, e.g. from below: ``` return Error( "Dynamically reserved resource " + stringify(resource) + " cannot be created from revocable resources"); ``` src/master/validation.cpp Lines 615-616 (patched) <https://reviews.apache.org/r/60013/#comment251411> Ditto here, no period / backticks. Also could you put the space on the next line to match our other logging? (We started doing this because at the end of the line it's less obvious when we missed it). src/master/validation.cpp Line 1207 (original), 1258 (patched) <https://reviews.apache.org/r/60013/#comment251414> We only passed the framework for validateUniqueTaskID because we needed to introspect the state (i.e. tasks), whereas here we really just need the FrameworkInfo or capabilities? - Benjamin Mahler On June 13, 2017, 8:34 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60013/ > ----------------------------------------------------------- > > (Updated June 13, 2017, 8:34 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > ------- > > Resources: Updated the validation logic. > > > Diffs > ----- > > src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 > src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 > src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 > src/tests/master_validation_tests.cpp > 4e7ce74edf0069b9317f869b299694a45e2a62f2 > src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d > > > Diff: https://reviews.apache.org/r/60013/diff/2/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
