You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Adam B <ad...@mesosphere.io> on 2015/07/07 03:30:10 UTC
Re: Review Request 35986: Allow slave attributes flag take a value
with ':'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35986/#review90623
-----------------------------------------------------------
Very good, but there are some subtleties that we should not ignore regarding empty names/values and multiple attributes with colons.
src/common/attributes.cpp (lines 150 - 152)
<https://reviews.apache.org/r/35986/#comment143744>
There's a subtle difference in behavior between strings::tokenize and strings::split. For tokenize, "Empty tokens will not be included in the result." whereas for split, "Empty tokens are allowed in the result." so you need to test not only that `pairs.size() == 2`, but also that `!pairs[0].empty()` and `!pairs[1].empty()`.
Let's create unit tests for handling `":foo"` and `"foo:"`.
src/tests/attributes_tests.cpp (line 55)
<https://reviews.apache.org/r/35986/#comment143738>
Would love to see this test multiple attributes, so we can be sure that subsequent colons still work as expected in situations like `parse("attr1:foo:bar;attr2:baz:qux")`.
- Adam B
On June 28, 2015, 6:49 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35986/
> -----------------------------------------------------------
>
> (Updated June 28, 2015, 6:49 a.m.)
>
>
> Review request for mesos and Isabel Jimenez.
>
>
> Bugs: MESOS-2868
> https://issues.apache.org/jira/browse/MESOS-2868
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allow slave attributes flag take a value with ':'.
>
>
> Diffs
> -----
>
> src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21
> src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7
>
> Diff: https://reviews.apache.org/r/35986/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35986: Allow slave attributes flag take a value
with ':'.
Posted by haosdent huang <ha...@gmail.com>.
> On July 7, 2015, 1:30 a.m., Adam B wrote:
> > src/common/attributes.cpp, lines 150-152
> > <https://reviews.apache.org/r/35986/diff/1/?file=994137#file994137line150>
> >
> > There's a subtle difference in behavior between strings::tokenize and strings::split. For tokenize, "Empty tokens will not be included in the result." whereas for split, "Empty tokens are allowed in the result." so you need to test not only that `pairs.size() == 2`, but also that `!pairs[0].empty()` and `!pairs[1].empty()`.
> > Let's create unit tests for handling `":foo"` and `"foo:"`.
Hi, @adam-mesos, could not add test unit tests for handling ":foo" and "foo:". Because use LOG(FATAL) in parse.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35986/#review90623
-----------------------------------------------------------
On July 7, 2015, 5:55 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35986/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 5:55 p.m.)
>
>
> Review request for mesos, Adam B and Isabel Jimenez.
>
>
> Bugs: MESOS-2868
> https://issues.apache.org/jira/browse/MESOS-2868
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allow slave attributes flag take a value with ':'.
>
>
> Diffs
> -----
>
> src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21
> src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7
>
> Diff: https://reviews.apache.org/r/35986/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>