You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/12/12 15:30:01 UTC
Re: Review Request 35711: Disallow special characters in role name.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Dec. 12, 2015, 2:29 p.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Address comments
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
include/mesos/roles.hpp PRE-CREATION
src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211
src/common/roles.cpp PRE-CREATION
src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806
src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399
src/tests/roles_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
make -j8 check
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review110341
-----------------------------------------------------------
Looking good! Minor nit below.
src/tests/roles_tests.cpp (line 55)
<https://reviews.apache.org/r/35711/#comment170151>
Grammar check: should be something like "Keep in mind that `Roles::validate` returns `Option<Error>`, so it will return `None()` when validation succeeds, or `Error()` when validation fails."
- Greg Mann
On Dec. 12, 2015, 2:33 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2015, 2:33 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
> -----
>
> include/mesos/roles.hpp PRE-CREATION
> src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
> src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
> src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211
> src/common/roles.cpp PRE-CREATION
> src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806
> src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399
> src/tests/roles_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 5, 2016, 5:51 a.m., Guangya Liu wrote:
> > include/mesos/roles.hpp, lines 29-49
> > <https://reviews.apache.org/r/35711/diff/19/?file=1181481#file1181481line29>
> >
> > It is better use /**/ for comments in a header file.
>
> haosdent huang wrote:
> According resources.hpp, seems it is OK here. https://github.com/apache/mesos/blob/master/include/mesos/resources.hpp#L166
>
> Guangya Liu wrote:
> The final goal should be /**/, please refer to https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md for detail
Thank you very much. Already updated, could you help review again?
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112771
-----------------------------------------------------------
On Jan. 5, 2016, 6:58 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 6:58 a.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
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 5, 2016, 5:51 a.m., Guangya Liu wrote:
> > include/mesos/roles.hpp, lines 29-49
> > <https://reviews.apache.org/r/35711/diff/19/?file=1181481#file1181481line29>
> >
> > It is better use /**/ for comments in a header file.
>
> haosdent huang wrote:
> According resources.hpp, seems it is OK here. https://github.com/apache/mesos/blob/master/include/mesos/resources.hpp#L166
The final goal should be /**/, please refer to https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md for detail
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112771
-----------------------------------------------------------
On 一月 5, 2016, 2:48 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated 一月 5, 2016, 2:48 a.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
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 5, 2016, 5:51 a.m., Guangya Liu wrote:
> > include/mesos/roles.hpp, lines 29-49
> > <https://reviews.apache.org/r/35711/diff/19/?file=1181481#file1181481line29>
> >
> > It is better use /**/ for comments in a header file.
>
> haosdent huang wrote:
> According resources.hpp, seems it is OK here. https://github.com/apache/mesos/blob/master/include/mesos/resources.hpp#L166
>
> Guangya Liu wrote:
> The final goal should be /**/, please refer to https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md for detail
>
> haosdent huang wrote:
> Thank you very much. Already updated, could you help review again?
Thanks for the update ;-)
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112771
-----------------------------------------------------------
On 一月 5, 2016, 6:58 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated 一月 5, 2016, 6:58 a.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
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 5, 2016, 5:51 a.m., Guangya Liu wrote:
> > include/mesos/roles.hpp, lines 29-49
> > <https://reviews.apache.org/r/35711/diff/19/?file=1181481#file1181481line29>
> >
> > It is better use /**/ for comments in a header file.
According resources.hpp, seems it is OK here. https://github.com/apache/mesos/blob/master/include/mesos/resources.hpp#L166
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112771
-----------------------------------------------------------
On Jan. 5, 2016, 2:48 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 2:48 a.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
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112771
-----------------------------------------------------------
include/mesos/roles.hpp (lines 29 - 49)
<https://reviews.apache.org/r/35711/#comment173264>
It is better use /**/ for comments in a header file.
- Guangya Liu
On 一月 5, 2016, 2:48 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated 一月 5, 2016, 2:48 a.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
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112878
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35711]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 5, 2016, 3:19 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 3:19 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/role_tests.cpp 91a6d27158a21ae11dd310dd1f55922dc147e6f3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 7, 2016, 10:51 p.m., Adam B wrote:
> > Still wondering if `class Roles` is any better than `namespace roles`, and unsure about the need for the CMake change.
Thank you very much for your great reviews! Could you help review again? I change to use namespaces roles.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113287
-----------------------------------------------------------
On Jan. 9, 2016, 11:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 11:15 a.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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113287
-----------------------------------------------------------
Still wondering if `class Roles` is any better than `namespace roles`, and unsure about the need for the CMake change.
include/mesos/roles.hpp (line 57)
<https://reviews.apache.org/r/35711/#comment173816>
s/Role name list/List of role names/
docs/roles.md (lines 105 - 106)
<https://reviews.apache.org/r/35711/#comment173879>
Shouldn't we use backticks instead of single quotes?
include/mesos/roles.hpp (lines 26 - 30)
<https://reviews.apache.org/r/35711/#comment173880>
This comment should go below the `namespace mesos {` line, just above `class Roles`.
include/mesos/roles.hpp (line 33)
<https://reviews.apache.org/r/35711/#comment173892>
This isn't much of a real 'class' anymore, is it? Any reason why you don't want to just wrap these functions in a `namespace roles {` instead?
include/mesos/roles.hpp (line 44)
<https://reviews.apache.org/r/35711/#comment173881>
Start this comment with a brief description, like "Validates the given role name. A role name..."
include/mesos/roles.hpp (line 50)
<https://reviews.apache.org/r/35711/#comment173883>
@return Error if validation fails, None otherwise.
include/mesos/roles.hpp (line 55)
<https://reviews.apache.org/r/35711/#comment173882>
s/roles object/list of roles/
src/CMakeLists.txt (line 104)
<https://reviews.apache.org/r/35711/#comment173885>
I think CMake currently only works for the agent build target, and roles.cpp is only used by master. Did you test if the cmake build works with/without this change?
We may need to pull in Alex Clemmer for advice.
src/tests/role_tests.cpp (lines 587 - 588)
<https://reviews.apache.org/r/35711/#comment173887>
EXPECT_SOME_EQ(v, r1.get())
Combine here and elsewhere
src/tests/role_tests.cpp (line 615)
<https://reviews.apache.org/r/35711/#comment173888>
Could you add another check that validate("foo..bar") and/or ".foo" succeeds, so we know we're only erroring on "." and ".." as full strings.
- Adam B
On Jan. 5, 2016, 7:19 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 7:19 a.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/role_tests.cpp 91a6d27158a21ae11dd310dd1f55922dc147e6f3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 14, 2016, 1:33 a.m., Adam B wrote:
> > Looks great! Just a few minor items that I can fix myself. I'll commit this shortly.
>
> haosdent huang wrote:
> Thank you very much!
No sir, thank you!
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review114435
-----------------------------------------------------------
On Jan. 13, 2016, 10 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10 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/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 14, 2016, 9:33 a.m., Adam B wrote:
> > Looks great! Just a few minor items that I can fix myself. I'll commit this shortly.
Thank you very much!
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review114435
-----------------------------------------------------------
On Jan. 14, 2016, 6 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 6 a.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/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review114435
-----------------------------------------------------------
Ship it!
Looks great! Just a few minor items that I can fix myself. I'll commit this shortly.
src/common/roles.cpp (line 58)
<https://reviews.apache.org/r/35711/#comment175280>
s/out this/this out/
s/satisfies/satisfy/
src/master/master.cpp (lines 643 - 644)
<https://reviews.apache.org/r/35711/#comment175281>
`LOG(WARNING) << "Duplicate values in '--roles': " << flags.roles.get();`
include/mesos/roles.hpp (line 36)
<https://reviews.apache.org/r/35711/#comment175284>
Need double blank lines between these, since they're at the top level (not within a class).
src/common/roles.cpp (line 30)
<https://reviews.apache.org/r/35711/#comment175285>
s/to support/supporting/
src/common/roles.cpp (line 68)
<https://reviews.apache.org/r/35711/#comment175287>
We can pull this (and its comment) out into a `static const string INVALID_CHARACTERS` in the namespace.
- Adam B
On Jan. 13, 2016, 10 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10 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/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review114439
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35711]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 14, 2016, 6 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 6 a.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/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 14, 2016, 6 a.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Rebase
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
docs/roles.md af1adad7ec8122fd10f7de44848014b850416bcd
include/mesos/roles.hpp PRE-CREATION
src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
src/common/roles.cpp PRE-CREATION
src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
make -j8 check
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review114367
-----------------------------------------------------------
Ship it!
Ship It!
- Klaus Ma
On Jan. 14, 2016, 1:12 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 1:12 a.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/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 13, 2016, 5:12 p.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Address @klaus1982 and @adam-mesos comments.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
docs/roles.md af1adad7ec8122fd10f7de44848014b850416bcd
include/mesos/roles.hpp PRE-CREATION
src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
src/common/roles.cpp PRE-CREATION
src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
make -j8 check
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 10, 2016, 7:44 a.m., Klaus Ma wrote:
> >
@klaus1982 @adam-mesos Thank you very much for your detail reviews. Could you help review again? Thank you very much.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113639
-----------------------------------------------------------
On Jan. 13, 2016, 5:12 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 5:12 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/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp c122c30d943813fc3ce9e7025783c7231809b022
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote:
> > src/common/roles.cpp, line 32
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line32>
> >
> > Do we need to define a const value `REOLE_SPLITOR` to replace ','?
>
> Adam B wrote:
> I think it's unnecessary, since this is the only place it's used. If we ever decide to make this configurable, we can break it out then.
Now that I think about it, we could even just inline `parse()` into the one place it's used in `master.cpp`, since it's really just a `tokenize()` followed by a `validate()`. Up to you, haosdent. I'm fine with leaving `parse()` alone too.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113639
-----------------------------------------------------------
On Jan. 9, 2016, 3:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 3:15 a.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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 10, 2016, 7:44 a.m., Klaus Ma wrote:
> > src/common/roles.cpp, lines 56-63
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line56>
> >
> > Just another question about the role: do we support other language, e.g. Chinese? If not, I'd suggest also to highlight in document.
>
> Adam B wrote:
> Good question. We're also likely have different requirements for other OS's like Windows or BSD. But all we really care about from a technical perspective is that the role name can be used to create a valid directory name on any agent. Remember that roles are set/validated on the master, but the agents are where we're creating per-role checkpoint directories, and each agent could have a different OS/locale. We really need a subset that satisfies all OS/locale constraints. Maybe this should be pulled out into a stout:: method (could be done as a follow-up patch).
yes, I also use isspace in the init patch. But because we also don't allow control characters, I change to use current way. http://en.cppreference.com/w/cpp/string/byte/isspace
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113639
-----------------------------------------------------------
On Jan. 9, 2016, 11:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 11:15 a.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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote:
> > src/common/roles.cpp, line 32
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line32>
> >
> > Do we need to define a const value `REOLE_SPLITOR` to replace ','?
I think it's unnecessary, since this is the only place it's used. If we ever decide to make this configurable, we can break it out then.
> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote:
> > src/common/roles.cpp, line 34
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line34>
> >
> > Do we need to check whether duplicated roles here?
Not necessary here, since they're de-duped when inserted into the hashmap in master.cpp.
Please also note that the roleWhitelist (`--roles`) is deprecating, and this function will be removed with it.
> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote:
> > src/common/roles.cpp, lines 56-63
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line56>
> >
> > Just another question about the role: do we support other language, e.g. Chinese? If not, I'd suggest also to highlight in document.
Good question. We're also likely have different requirements for other OS's like Windows or BSD. But all we really care about from a technical perspective is that the role name can be used to create a valid directory name on any agent. Remember that roles are set/validated on the master, but the agents are where we're creating per-role checkpoint directories, and each agent could have a different OS/locale. We really need a subset that satisfies all OS/locale constraints. Maybe this should be pulled out into a stout:: method (could be done as a follow-up patch).
> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote:
> > include/mesos/roles.hpp, line 51
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189195#file1189195line51>
> >
> > Would you highlight which case is considered not valid? anyone of them is not valid or all of them are not valid?
Should be: "Returns Error if any role is invalid."
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113639
-----------------------------------------------------------
On Jan. 9, 2016, 3:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 3:15 a.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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 10, 2016, 3:44 p.m., Klaus Ma wrote:
> > src/common/roles.cpp, line 34
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line34>
> >
> > Do we need to check whether duplicated roles here?
>
> Adam B wrote:
> Not necessary here, since they're de-duped when inserted into the hashmap in master.cpp.
> Please also note that the roleWhitelist (`--roles`) is deprecating, and this function will be removed with it.
Yes, but I'd suggest to log a warning messoage for cluster admin; for example, if Operator set duplidated role by mistake, it's hard for him to identify where is the issue.
> On Jan. 10, 2016, 3:44 p.m., Klaus Ma wrote:
> > src/common/roles.cpp, lines 56-63
> > <https://reviews.apache.org/r/35711/diff/22/?file=1189198#file1189198line56>
> >
> > Just another question about the role: do we support other language, e.g. Chinese? If not, I'd suggest also to highlight in document.
>
> Adam B wrote:
> Good question. We're also likely have different requirements for other OS's like Windows or BSD. But all we really care about from a technical perspective is that the role name can be used to create a valid directory name on any agent. Remember that roles are set/validated on the master, but the agents are where we're creating per-role checkpoint directories, and each agent could have a different OS/locale. We really need a subset that satisfies all OS/locale constraints. Maybe this should be pulled out into a stout:: method (could be done as a follow-up patch).
>
> haosdent huang wrote:
> yes, I also use isspace in the init patch. But because we also don't allow control characters, I change to use current way. http://en.cppreference.com/w/cpp/string/byte/isspace
Agree with Adam to pulled this part out into `stout`; that'll be helpful for us to support multiple OS/locale.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113639
-----------------------------------------------------------
On Jan. 9, 2016, 7:15 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 7:15 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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113639
-----------------------------------------------------------
include/mesos/roles.hpp (line 51)
<https://reviews.apache.org/r/35711/#comment174396>
Would you highlight which case is considered not valid? anyone of them is not valid or all of them are not valid?
src/common/roles.cpp (line 32)
<https://reviews.apache.org/r/35711/#comment174400>
Do we need to define a const value `REOLE_SPLITOR` to replace ','?
src/common/roles.cpp (line 34)
<https://reviews.apache.org/r/35711/#comment174397>
Do we need to check whether duplicated roles here?
src/common/roles.cpp (lines 56 - 63)
<https://reviews.apache.org/r/35711/#comment174399>
Just another question about the role: do we support other language, e.g. Chinese? If not, I'd suggest also to highlight in document.
src/common/roles.cpp (line 64)
<https://reviews.apache.org/r/35711/#comment174398>
I'm not sure whether this will introduce porting issue: for the whitespace, it's better to use C std (isspace) lib to check.
- Klaus Ma
On Jan. 9, 2016, 7:15 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 7:15 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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review113644
-----------------------------------------------------------
Ship it!
Looks good to me. We can commit this once you've resolved any remaining issues (from Klaus, etc.)
include/mesos/roles.hpp (line 33)
<https://reviews.apache.org/r/35711/#comment174402>
s/otherwise return/otherwise a/
include/mesos/roles.hpp (line 54)
<https://reviews.apache.org/r/35711/#comment174404>
"Error if validation fails for any role, None otherwise."
src/tests/role_tests.cpp (line 602)
<https://reviews.apache.org/r/35711/#comment174410>
s/function/functions/
- Adam B
On Jan. 9, 2016, 3:15 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2016, 3:15 a.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/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/common/roles.cpp PRE-CREATION
> src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 9, 2016, 11:15 a.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Address @adam comments.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
docs/roles.md af1adad7ec8122fd10f7de44848014b850416bcd
include/mesos/roles.hpp PRE-CREATION
src/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8
src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
src/common/roles.cpp PRE-CREATION
src/master/master.cpp 2d9b7f9540574aa3ef9e5af3b2b8922dffeebac8
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
make -j8 check
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 3:19 p.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Remove Roles class.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
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/role_tests.cpp 91a6d27158a21ae11dd310dd1f55922dc147e6f3
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
make -j8 check
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On Jan. 5, 2016, 8:16 a.m., Adam B wrote:
> > Looks good. My biggest question is whether `class Roles` needs to exist, or whether we can just use freestanding `roles::validate()` functions.
Thank you very much. I use vector<string> instead. Could you help review again?
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review112788
-----------------------------------------------------------
On Jan. 5, 2016, 3:19 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 3:19 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/role_tests.cpp 91a6d27158a21ae11dd310dd1f55922dc147e6f3
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 6:58 a.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Update comments style
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
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
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 2:48 a.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Update documents.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
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
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 2:42 a.m.)
Review request for mesos, Adam B, Jie Yu, and Michael Park.
Changes
-------
Address @adam and @greggomann comments.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
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
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
> On Dec. 15, 2015, 2:45 a.m., Adam B wrote:
> > Thanks for reviving this, and sorry it's taken so long to get back to it.
>
> Adam B wrote:
> You'll also need to add some documentation about what the valid/invalid role names are, and when they'll be rejected.
Could you add this info to docs/roles.md?
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review110461
-----------------------------------------------------------
On Dec. 12, 2015, 6:33 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2015, 6:33 a.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
> -----
>
> include/mesos/roles.hpp PRE-CREATION
> src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
> src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
> src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211
> src/common/roles.cpp PRE-CREATION
> src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806
> src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399
> src/tests/roles_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
> On Dec. 15, 2015, 2:45 a.m., Adam B wrote:
> > Thanks for reviving this, and sorry it's taken so long to get back to it.
You'll also need to add some documentation about what the valid/invalid role names are, and when they'll be rejected.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review110461
-----------------------------------------------------------
On Dec. 12, 2015, 6:33 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2015, 6:33 a.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
> -----
>
> include/mesos/roles.hpp PRE-CREATION
> src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
> src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
> src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211
> src/common/roles.cpp PRE-CREATION
> src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806
> src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399
> src/tests/roles_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review110461
-----------------------------------------------------------
Thanks for reviving this, and sorry it's taken so long to get back to it.
include/mesos/roles.hpp (line 43)
<https://reviews.apache.org/r/35711/#comment170388>
We may have additional constraints when we have to create directories in Windows based on role names.
Maybe @hausdorff can tell us more.
src/common/roles.cpp (lines 47 - 49)
<https://reviews.apache.org/r/35711/#comment170383>
Or empty string?
src/master/flags.cpp (lines 17 - 23)
<https://reviews.apache.org/r/35711/#comment170391>
Why did these includes need to be added?
src/master/master.hpp (line 31)
<https://reviews.apache.org/r/35711/#comment170392>
Why add the include?
src/slave/flags.cpp (line 24)
<https://reviews.apache.org/r/35711/#comment170396>
Why the include?
src/tests/roles_tests.cpp (line 55)
<https://reviews.apache.org/r/35711/#comment170401>
+1 to Greg's grammar check.
src/tests/roles_tests.cpp (line 57)
<https://reviews.apache.org/r/35711/#comment170399>
s/vaildate/validate/
src/tests/roles_tests.cpp (line 66)
<https://reviews.apache.org/r/35711/#comment170402>
Empty string should error.
- Adam B
On Dec. 12, 2015, 6:33 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2015, 6:33 a.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
> -----
>
> include/mesos/roles.hpp PRE-CREATION
> src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
> src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
> src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211
> src/common/roles.cpp PRE-CREATION
> src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806
> src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399
> src/tests/roles_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
> make -j8 check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/
-----------------------------------------------------------
(Updated Dec. 12, 2015, 2:33 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 (updated)
-----
include/mesos/roles.hpp PRE-CREATION
src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211
src/common/roles.cpp PRE-CREATION
src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806
src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399
src/tests/roles_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
make -j8 check
Thanks,
haosdent huang