----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51248/#review146403 -----------------------------------------------------------
Fix it, then Ship it! src/master/validation.cpp (lines 473 - 475) <https://reviews.apache.org/r/51248/#comment212836> Along with Guangya's suggestion for less jaggedness, could you put a newline between comment sections? i.e. ``` // Validates that revocable and non-revocable // resource of the same name do not exist. // // TODO(vinod): Is this the right place to do this? ``` src/master/validation.cpp (line 476) <https://reviews.apache.org/r/51248/#comment212840> s/ </</ src/master/validation.cpp (lines 482 - 483) <https://reviews.apache.org/r/51248/#comment212839> Do you want to quote the 'name' here? src/master/validation.cpp (line 541) <https://reviews.apache.org/r/51248/#comment212841> FWICT we've been putting switch braces on the same line? src/master/validation.cpp (lines 556 - 557) <https://reviews.apache.org/r/51248/#comment212844> Maybe a comment here about backwards compatibility because at first glance it seems this should be invalid. Specifically, if a new type is introduced, we can't tell the difference between old unset type and new stripped type (both show up as unset and default to UNKNOWN here). Seems like this comment should perhaps be up in the protobufs as well? src/master/validation.cpp (lines 576 - 577) <https://reviews.apache.org/r/51248/#comment212848> This copies the whole executor map! How about: ``` executorInfo = slave->executors.at(framework->id()).at(executorId); ``` src/master/validation.cpp (line 761) <https://reviews.apache.org/r/51248/#comment212819> Hm.. is this function also documented in the header? I would suggest that we only document functions in the header if that's where they are declared (as that has been the style we follow). If we document in both places they are likely to go out of sync. src/master/validation.cpp (lines 790 - 791) <https://reviews.apache.org/r/51248/#comment212820> Ditto here, only document this in the header? src/master/validation.cpp (line 821) <https://reviews.apache.org/r/51248/#comment212821> Ditto here, only document in header? src/master/validation.cpp (lines 830 - 831) <https://reviews.apache.org/r/51248/#comment212824> Did we lose the comment about why the order matters? Are there too many ordering assumptions to mention? src/master/validation.cpp (line 854) <https://reviews.apache.org/r/51248/#comment212822> Ditto here, only document in header? src/master/validation.cpp (lines 883 - 893) <https://reviews.apache.org/r/51248/#comment212852> TODO here to reflect how this will need to be revisited in the future? src/master/validation.cpp (line 897) <https://reviews.apache.org/r/51248/#comment212854> Do you need this variable? I wonder if it's more readable without it. src/master/validation.cpp (lines 928 - 931) <https://reviews.apache.org/r/51248/#comment212838> Ditto for less jaggedness and a newline before the NOTE section. src/master/validation.cpp (line 949) <https://reviews.apache.org/r/51248/#comment212823> Ditto here, only document in header? src/tests/master_validation_tests.cpp (line 705) <https://reviews.apache.org/r/51248/#comment212815> Seems like there should be a comment mentioning that this test omits the executor 'type'? src/tests/master_validation_tests.cpp (line 720) <https://reviews.apache.org/r/51248/#comment212816> Perhaps call out that we don't set the 'type' here, which is the "old" way of specifying the executor? src/tests/master_validation_tests.cpp (line 744) <https://reviews.apache.org/r/51248/#comment212818> I'm a bit confused about what this is testing, should we add a comment? I'm guessing that this is testing that the DEFAULT executor is not supported yet and so we validate that it is rejected here? If so, seems we need a TODO at the top of this test to mention that this will need to be re-visited? src/tests/master_validation_tests.cpp (lines 1407 - 1421) <https://reviews.apache.org/r/51248/#comment212817> This could be picking up errors other than the one's you're expecting, should you do a 'contains' check for the error message? Also, it would be good to have the positive cases here as well (should get None if CUSTOM and you do have command, or DEFAULT and no command). - Benjamin Mahler On Aug. 21, 2016, 11:28 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51248/ > ----------------------------------------------------------- > > (Updated Aug. 21, 2016, 11:28 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6042 > https://issues.apache.org/jira/browse/MESOS-6042 > > > Repository: mesos > > > Description > ------- > > Refactored in such a way that most of the helper functions can be > reused for doing task group validation. > > Note that there are no functional changes here only code movement. > > > Diffs > ----- > > src/master/validation.hpp fd00609df483930d9f749ce764830129da9e8a2c > src/master/validation.cpp ddc7ac3965c93138a38fe3c79776173c610393d6 > src/tests/master_validation_tests.cpp > ad89812a55fa56c2e13e2a683e35f777dc6341a9 > > Diff: https://reviews.apache.org/r/51248/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
