----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18339/#review35672 -----------------------------------------------------------
I think this review is doing too much than what its description says. I would prefer to split this in to atleast 2 or 3 patches, one for validation, one for s/visitor/validator/, and maybe one for changing semantics (const *, control flow etc). src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66350> There are comments in this code that refer to the "visitor" pattern. You should change those to "validator" pattern. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66348> new line. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66349> s/, Ie,/, i.e.,/ ? src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66352> The formatting style is to have one arg per line. virtual TaskInfoError run( const foo, const bar) src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66355> How about "Task ID '" + id + "' contains invalid characters. ID must be alphanumeric or '_' or '-'"; src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66359> I would prefer to be less strict so that we dont unintentionally break frameworks. how about: return iscntrl(c) || c == "/"; src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66353> ditto. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66361> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66360> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66362> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66363> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66364> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66365> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66366> formatting. src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66372> We don't use const pointers in our code base (that often). Can you revert? src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66374> Why the change from functor to "run" ? src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66373> s/visitor/validator/ src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66368> s/Visitors/Validators/ src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66375> s/visitor/validator/ src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66376> I think the previous logic was easier to follow. Why the change? src/master/master.cpp <https://reviews.apache.org/r/18339/#comment66377> We typically don't do const pointers. So revert? - Vinod Kone On Feb. 21, 2014, 6:21 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18339/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2014, 6:21 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.hpp 9d1b56c6b02eb21130f165848297ae0695ac2af7 > src/master/master.cpp cb46869cd298f3a4fcbe8e4e3fea4bb7c741a0e0 > > Diff: https://reviews.apache.org/r/18339/diff/ > > > Testing > ------- > > built. ran make check. ran local master/slave/python framework. > > > Thanks, > > Dominic Hamon > >
