----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18339/#review36321 -----------------------------------------------------------
Hey Dominic, I have a few comments below, can you follow up? Also, would love to see a follow up patch that tests this! src/master/master.cpp <https://reviews.apache.org/r/18339/#comment67277> Remove std:: qualifier. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment67278> Even if id.size() < PATH_MAX, the ID may be too long, so a TODO here to reflect that this is not sufficient would be good. Also, the PATH_MAX could be different on the slave machines, or on _each_ slave. We should ultimately correctly handle a mkdir failure in the slave (there was a ticket raised for this). I'd like to avoid doing this check in favor of handling it in the slave due to the inherit issue of checking PATH_MAX: http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html Also, you would need <limits.h> for PATH_MAX. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment67279> Can't you just use std::any_of here? src/master/master.cpp <https://reviews.apache.org/r/18339/#comment67280> How about s/isInvalid/invalid/ s/int/char/ src/master/master.cpp <https://reviews.apache.org/r/18339/#comment67281> You need <cctype> for iscntrl. - Ben Mahler On March 4, 2014, 8:42 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18339/ > ----------------------------------------------------------- > > (Updated March 4, 2014, 8:42 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-361 > https://issues.apache.org/jira/browse/MESOS-361 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > It would be good to have specific unit tests for these validators. I could > pull out the validation to another file and add unit tests on them. > > Final validation for TaskID is awaiting consensus (or as close as we can get) > on bug/mailing list. I do prefer to be conservative but I don't want to break > users. > > > Diffs > ----- > > src/master/master.cpp 2e86a1903ce0a0b461eae77078177cc7d28a2659 > > Diff: https://reviews.apache.org/r/18339/diff/ > > > Testing > ------- > > built. ran make check. ran local master/slave/python framework. > > > Thanks, > > Dominic Hamon > >
