mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 35711: Disallow special characters in role name.
Date Sun, 10 Jan 2016 09:45:53 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message