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/06/21 16:08:48 UTC
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/
-----------------------------------------------------------
Review request for mesos, Adam B and Jie Yu.
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On June 24, 2015, 9:30 a.m., Adam B wrote:
> > Looks great! We recently added validation lambdas to flag definitions, so you may be able to take advantage of that.
> > We probably also want to do validation in the slave for the --resources flag, since resources can be declared as reserved for particular roles, and reserving a resource for an invalid role would be bad. I don't know if we even validate the reservation roles as known roles in the master upon slave registration. The only difference with --resources is that resources can use `*` to declare resources as unreserved.
I check the "Master::registerSlave", master don't check reservation roles. So I add this check to `src/slave/flags.cpp`.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review89140
-----------------------------------------------------------
On June 27, 2015, 8:36 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 8:36 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/validation.hpp 469d6f56c3de28a34177124aae81ce24cb4ad160
> src/master/validation.cpp 9d128aa1b349b018b8e4a1916434d848761ca051
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
>
> 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/#review89140
-----------------------------------------------------------
Looks great! We recently added validation lambdas to flag definitions, so you may be able to take advantage of that.
We probably also want to do validation in the slave for the --resources flag, since resources can be declared as reserved for particular roles, and reserving a resource for an invalid role would be bad. I don't know if we even validate the reservation roles as known roles in the master upon slave registration. The only difference with --resources is that resources can use `*` to declare resources as unreserved.
src/master/master.hpp (lines 54 - 55)
<https://reviews.apache.org/r/35711/#comment141738>
Alphabetize, please
src/master/master.hpp (line 632)
<https://reviews.apache.org/r/35711/#comment141743>
We should also disallow role name `*` in master --roles because it is used to indicate unreserved resources.
src/master/master.hpp (line 635)
<https://reviews.apache.org/r/35711/#comment141740>
"Role name 'foobar' is invalid because it starts with a dash."
src/master/master.hpp (lines 638 - 639)
<https://reviews.apache.org/r/35711/#comment141741>
Why not use string::find()?
src/master/master.hpp (line 641)
<https://reviews.apache.org/r/35711/#comment141742>
How about "Role name 'foobar' is invalid because it contains xyz."? (here and below)
src/master/master.cpp (line 558)
<https://reviews.apache.org/r/35711/#comment141739>
Consider using the new flags validation lambdas as added by https://reviews.apache.org/r/34943
I know it will require you to tokenize/iterate through the roles twice, but that's probably not much of a performance hit.
- Adam B
On June 21, 2015, 7:21 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 21, 2015, 7:21 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
>
> 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 June 30, 2015, 4:56 p.m.)
Review request for mesos, Adam B and Jie Yu.
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/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/common/roles.cpp PRE-CREATION
src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
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 June 30, 2015, 8:30 a.m.)
Review request for mesos, Adam B and Jie Yu.
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/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/common/roles.cpp PRE-CREATION
src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
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 June 30, 2015, 8:29 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
CHANGELOG 252f068383308c5f07a0fced8cda954f288c9591
docs/mesos-markdown-style-guide.md fe9ffefbbb2c0ce9ade39cec1de073163a9a03b2
docs/upgrades.md cef055b95ac1167fcc5f0f7251fafda113840eda
include/mesos/mesos.proto 5ab3c4a305562af16af804e7ab77e576b96e468d
include/mesos/roles.hpp PRE-CREATION
src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8
src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/common/roles.cpp PRE-CREATION
src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
src/exec/exec.cpp a1ae074b962d8e93ab7776bd624389857da486f3
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 34b8acb6512819ed89a737cdcc6c516eafb44cbe
src/java/mesos.pom.in 7615d61eb6fedfa0ead785cd360946c56ccf80af
src/java/src/org/apache/mesos/MesosExecutorDriver.java 6741b3239daa435045ab01f830cf495dd2e65a3f
src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
src/sched/sched.cpp 7563abb85819b0b2bc9afdfd810b33c923c2522e
src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0
src/slave/status_update_manager.cpp 1978ac8ab370f3e20f5c3c803beda468edf31e2c
src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e
src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db
src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488
src/tests/partition_tests.cpp a1fb6f730e4b28758a8f03ad31f463b115194a6e
src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
src/tests/roles_tests.cpp PRE-CREATION
src/tests/slave_tests.cpp 4ddc608ab9636fcc0166e8c80a252dcf67b45ad3
src/webui/master/static/browse.html 0904c879de9cd602f23d749ad40ff56d5a2b0770
support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351
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 June 30, 2015, 7:53 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
CHANGELOG 252f068383308c5f07a0fced8cda954f288c9591
docs/mesos-markdown-style-guide.md fe9ffefbbb2c0ce9ade39cec1de073163a9a03b2
docs/upgrades.md cef055b95ac1167fcc5f0f7251fafda113840eda
include/mesos/mesos.proto 5ab3c4a305562af16af804e7ab77e576b96e468d
include/mesos/roles.hpp PRE-CREATION
src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
src/common/protobuf_utils.hpp afe5a85d3f58eaabb16807253c5fcc07cabcf8e8
src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/common/roles.cpp PRE-CREATION
src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
src/exec/exec.cpp a1ae074b962d8e93ab7776bd624389857da486f3
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 34b8acb6512819ed89a737cdcc6c516eafb44cbe
src/java/mesos.pom.in 7615d61eb6fedfa0ead785cd360946c56ccf80af
src/java/src/org/apache/mesos/MesosExecutorDriver.java 6741b3239daa435045ab01f830cf495dd2e65a3f
src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
src/sched/sched.cpp 7563abb85819b0b2bc9afdfd810b33c923c2522e
src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0
src/slave/status_update_manager.cpp 1978ac8ab370f3e20f5c3c803beda468edf31e2c
src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e
src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db
src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488
src/tests/partition_tests.cpp a1fb6f730e4b28758a8f03ad31f463b115194a6e
src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
src/tests/roles_tests.cpp PRE-CREATION
src/tests/slave_tests.cpp 4ddc608ab9636fcc0166e8c80a252dcf67b45ad3
src/webui/master/static/browse.html 0904c879de9cd602f23d749ad40ff56d5a2b0770
support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351
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/#review89626
-----------------------------------------------------------
src/Makefile.am (line 362)
<https://reviews.apache.org/r/35711/#comment142280>
Here are use spaces before, so I replace spaces to tabs.
- haosdent huang
On June 27, 2015, 11:35 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 11:35 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
> src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
> src/common/validation.hpp PRE-CREATION
> src/common/validation.cpp PRE-CREATION
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
> src/tests/common/validation_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
>
> 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/#review89627
-----------------------------------------------------------
src/Makefile.am (line 1489)
<https://reviews.apache.org/r/35711/#comment142281>
I also replace spaces to tabs here.
- haosdent huang
On June 27, 2015, 11:35 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 11:35 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
> src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
> src/common/validation.hpp PRE-CREATION
> src/common/validation.cpp PRE-CREATION
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
> src/tests/common/validation_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
>
> 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 Jie Yu <yu...@gmail.com>.
> On June 29, 2015, 11:17 a.m., Adam B wrote:
> > src/common/validation.hpp, lines 27-34
> > <https://reviews.apache.org/r/35711/diff/7/?file=993941#file993941line27>
> >
> > I'm not convinced this is the right namespace/scope for a freestanding validate() function. Maybe, like we have a Resource protobuf message and a Resources class with parse() and validate(), we should also have a Roles::validate().
+1
Initially, the patch only contains validation at the master's side, and that's the reason I sugguested to put that into master/validation.hpp|cpp. I agree with Adam that putting that in Roles. Sorry about that!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review89630
-----------------------------------------------------------
On June 27, 2015, 11:35 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 11:35 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
> src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
> src/common/validation.hpp PRE-CREATION
> src/common/validation.cpp PRE-CREATION
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
> src/tests/common/validation_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
>
> 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 June 29, 2015, 11:17 a.m., Adam B wrote:
> > src/common/validation.hpp, lines 27-34
> > <https://reviews.apache.org/r/35711/diff/7/?file=993941#file993941line27>
> >
> > I'm not convinced this is the right namespace/scope for a freestanding validate() function. Maybe, like we have a Resource protobuf message and a Resources class with parse() and validate(), we should also have a Roles::validate().
>
> Jie Yu wrote:
> +1
>
> Initially, the patch only contains validation at the master's side, and that's the reason I sugguested to put that into master/validation.hpp|cpp. I agree with Adam that putting that in Roles. Sorry about that!
Thank you for your review. Let me update it. @jieyu @adam-mesos
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review89630
-----------------------------------------------------------
On June 27, 2015, 11:35 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 11:35 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
> src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
> src/common/validation.hpp PRE-CREATION
> src/common/validation.cpp PRE-CREATION
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
> src/tests/common/validation_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
>
> 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/#review89630
-----------------------------------------------------------
Looks great. I think you're covering all the places where roles are specified now. But I'd like to see the validate() method move onto a Role/Roles class somewhere like Resources, rather than freestanding in an internal namespace.
src/common/validation.hpp (lines 27 - 34)
<https://reviews.apache.org/r/35711/#comment142465>
I'm not convinced this is the right namespace/scope for a freestanding validate() function. Maybe, like we have a Resource protobuf message and a Resources class with parse() and validate(), we should also have a Roles::validate().
src/common/validation.cpp (lines 20 - 26)
<https://reviews.apache.org/r/35711/#comment142489>
Unused?
src/common/validation.cpp (lines 43 - 44)
<https://reviews.apache.org/r/35711/#comment142285>
I suppose role name ".." would also be invalid by similar logic (cannot `mkdir ..`)
src/common/validation.cpp (lines 50 - 55)
<https://reviews.apache.org/r/35711/#comment142468>
s/space characters/whitespace/
src/common/validation.cpp (line 58)
<https://reviews.apache.org/r/35711/#comment142491>
What is `\x20`? You didn't comment it.
src/common/validation.cpp (line 61)
<https://reviews.apache.org/r/35711/#comment142467>
s/space/whitespace/
src/master/flags.cpp (line 189)
<https://reviews.apache.org/r/35711/#comment142493>
This could be a quick Roles::parse()
src/master/flags.cpp (lines 194 - 196)
<https://reviews.apache.org/r/35711/#comment142471>
Check for the `*` first, then the other check can just `return Role::validate(role);`.
src/slave/flags.cpp (line 52)
<https://reviews.apache.org/r/35711/#comment142474>
s/value/resourcesString/ since 'value' is referenced as something else in the help string above.
src/slave/flags.cpp (lines 61 - 62)
<https://reviews.apache.org/r/35711/#comment142477>
How about `return Resources::validate(parsed.get());`
src/tests/common/validation_tests.cpp (lines 38 - 40)
<https://reviews.apache.org/r/35711/#comment142482>
Where are these being used?
src/tests/common/validation_tests.cpp (line 50)
<https://reviews.apache.org/r/35711/#comment142484>
s/name/names/
s/contains/contain/
src/tests/common/validation_tests.cpp (lines 59 - 60)
<https://reviews.apache.org/r/35711/#comment142480>
Please add tests for strings starting with and strings containing a `-`.
src/tests/common/validation_tests.cpp (line 63)
<https://reviews.apache.org/r/35711/#comment142481>
Test `\n` as well?
- Adam B
On June 27, 2015, 4:35 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 4:35 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
> src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
> src/common/validation.hpp PRE-CREATION
> src/common/validation.cpp PRE-CREATION
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
> src/tests/common/validation_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
>
> 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 June 27, 2015, 11:35 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/common/validation.hpp PRE-CREATION
src/common/validation.cpp PRE-CREATION
src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
src/tests/common/validation_tests.cpp PRE-CREATION
src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
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 June 27, 2015, 11:32 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/common/validation.hpp PRE-CREATION
src/common/validation.cpp PRE-CREATION
src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
src/tests/common/validation_tests.cpp PRE-CREATION
src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
Diff: https://reviews.apache.org/r/35711/diff/
Testing (updated)
-------
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 June 27, 2015, 8:36 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/validation.hpp 469d6f56c3de28a34177124aae81ce24cb4ad160
src/master/validation.cpp 9d128aa1b349b018b8e4a1916434d848761ca051
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 35711: Disallow special characters in role name.
Posted by haosdent huang <ha...@gmail.com>.
> On June 24, 2015, 6:34 p.m., Jie Yu wrote:
> > src/master/master.hpp, lines 630-651
> > <https://reviews.apache.org/r/35711/diff/4/?file=989196#file989196line630>
> >
> > Can you move this function to src/master/validation.hpp|cpp
> >
> > ```
> > namespace role {
> > Option<Error> validate(const std::string& role);
> > }
> > ```
Because both master and slave need this check, I create src/common/validation.hpp|cpp .
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review89222
-----------------------------------------------------------
On June 27, 2015, 11:35 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 11:35 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5
> src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
> src/common/validation.hpp PRE-CREATION
> src/common/validation.cpp PRE-CREATION
> src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8
> src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0
> src/tests/common/validation_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e
>
> 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 Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35711/#review89222
-----------------------------------------------------------
+1 on Adam's comments.
src/master/master.hpp (lines 630 - 651)
<https://reviews.apache.org/r/35711/#comment141817>
Can you move this function to src/master/validation.hpp|cpp
```
namespace role {
Option<Error> validate(const std::string& role);
}
```
src/master/master.hpp (lines 638 - 639)
<https://reviews.apache.org/r/35711/#comment141820>
Add a space after 'foreach'
- Jie Yu
On June 21, 2015, 2:21 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
>
> (Updated June 21, 2015, 2:21 p.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2210
> https://issues.apache.org/jira/browse/MESOS-2210
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Disallow special characters in role name.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
>
> Diff: https://reviews.apache.org/r/35711/diff/
>
>
> Testing
> -------
>
>
> 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 June 21, 2015, 2:21 p.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
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 June 21, 2015, 2:16 p.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
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 June 21, 2015, 2:15 p.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs (updated)
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
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 June 21, 2015, 2:09 p.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2210
https://issues.apache.org/jira/browse/MESOS-2210
Repository: mesos
Description
-------
Disallow special characters in role name.
Diffs
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
Diff: https://reviews.apache.org/r/35711/diff/
Testing
-------
Thanks,
haosdent huang