----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112788 -----------------------------------------------------------
Looks good. My biggest question is whether `class Roles` needs to exist, or whether we can just use freestanding `roles::validate()` functions. docs/roles.md (line 102) <https://reviews.apache.org/r/35711/#comment173279> A role name must be a valid directory name, so it cannot: * Be an empty string * Be '.' or '..' * Start with '-' * Contain any slash, backspace, or whitespace character include/mesos/roles.hpp (lines 29 - 31) <https://reviews.apache.org/r/35711/#comment173280> This comment seems like it should be a class comment just above `class Roles`, and you might as well doxygenize it like you did the method comments. include/mesos/roles.hpp (lines 47 - 51) <https://reviews.apache.org/r/35711/#comment173281> A role name must be a valid directory name, so it cannot: * Be an empty string * Be '.' or '..' * Start with '-' * Contain invalid characters (slash, backspace, or whitespace). include/mesos/roles.hpp (line 64) <https://reviews.apache.org/r/35711/#comment173284> How is any of the rest of this any different from just using `std::vector<std::string> roles` directly? Why not make Roles an abstract class with a few static methods, or remove the class altogether and just use free-standing methods within the mesos::roles namespace? And if you do need this class for some reason, do all of these methods need to be public? include/mesos/roles.hpp (line 74) <https://reviews.apache.org/r/35711/#comment173283> `s/operator = (/operator=(/` src/Makefile.am (lines 1760 - 1761) <https://reviews.apache.org/r/35711/#comment173286> What's the difference between `role_tests.cpp` and `roles_tests.cpp`? Can we combine them into one file? src/common/roles.cpp (lines 91 - 93) <https://reviews.apache.org/r/35711/#comment173287> This is a small enough section, I think you can do without the 5 extra lines this comment adds. src/master/master.cpp (line 559) <https://reviews.apache.org/r/35711/#comment173288> s/determine/parse/ src/tests/roles_tests.cpp (line 32) <https://reviews.apache.org/r/35711/#comment173291> s/This test tests/This tests/ src/tests/roles_tests.cpp (line 55) <https://reviews.apache.org/r/35711/#comment173292> s/This test tests/This tests/ - Adam B On Jan. 4, 2016, 10:58 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35711/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2016, 10:58 p.m.) > > > Review request for mesos, Adam B, Jie Yu, and Michael Park. > > > Bugs: MESOS-2210 > https://issues.apache.org/jira/browse/MESOS-2210 > > > Repository: mesos > > > Description > ------- > > Disallow special characters in role name. > > > Diffs > ----- > > docs/roles.md af1adad7ec8122fd10f7de44848014b850416bcd > include/mesos/roles.hpp PRE-CREATION > src/CMakeLists.txt 8169fe4f1a0eda5c2e2b30e22a7346478420a9a0 > src/Makefile.am e08e86724abaa3023f0483aa222354c95d4d3817 > src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4 > src/common/roles.cpp PRE-CREATION > src/master/master.cpp 40ce3e17fca88da689128bcf5d35fdddc396c011 > src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320 > src/tests/roles_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/35711/diff/ > > > Testing > ------- > > make -j8 check > > > Thanks, > > haosdent huang > >
