Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/#review167345
---



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On July 21, 2016, 4:34 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49223/
> ---
> 
> (Updated July 21, 2016, 4:34 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5739
> https://issues.apache.org/jira/browse/MESOS-5739
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Value parsing code to only accept the canonical formats.
> NOTES: This patch isn't doing a complete validation against
> the canonical format yet: the set entries is not validated as
> conforming to the Values.Text format yet.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 
> 
> 
> Diff: https://reviews.apache.org/r/49223/diff/8/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/
---

(Updated July 22, 2016, 12:34 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Handle empty Range & Set case.


Bugs: MESOS-5739
https://issues.apache.org/jira/browse/MESOS-5739


Repository: mesos


Description (updated)
---

Fixed Value parsing code to only accept the canonical formats.
NOTES: This patch isn't doing a complete validation against
the canonical format yet: the set entries is not validated as
conforming to the Values.Text format yet.


Diffs (updated)
-

  src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
  src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
  src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 

Diff: https://reviews.apache.org/r/49223/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/
---

(Updated July 21, 2016, 11:44 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Update depdencies.


Bugs: MESOS-5739
https://issues.apache.org/jira/browse/MESOS-5739


Repository: mesos


Description
---

Fixed Value parsing code to only accept the canonical formats.
NOTES: This patch isn't doing a complete validation against 
the canonical format yet: the set entries is not validated as
conforming to the Values.Text format yet.


Diffs
-

  src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
  src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
  src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 

Diff: https://reviews.apache.org/r/49223/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

> On July 21, 2016, 7:44 a.m., Benjamin Mahler wrote:
> > src/common/values.cpp, lines 589-594
> > 
> >
> > Do we still need this? Seems to me that we want to do the following:
> > 
> > ```
> > // Ranges. E.g. [1-2,4-5]
> > if (strings::startsWith(temp, "[")) {
> >   if (!strings::endsWith(temp, "]")) {
> > return Error("Missing a closing bracket");
> >   }
> > 
> >   ...
> > }
> > 
> > if (strings::startsWith(temp, "{")) {
> >   if (!strings::endsWith(temp, "}")) {
> > return Error("Missing a closing brace");
> >   }
> >   
> >   ..
> > }
> > ```
> > 
> > Also, why is this looking at parentheses? I only see brackets (ranges) 
> > and braces (set) being used.

I think this is not necessary. Removed.


> On July 21, 2016, 7:44 a.m., Benjamin Mahler wrote:
> > src/common/values.cpp, line 611
> > 
> >
> > Looks like you want a strings::split here? strings::tokenize will 
> > ignore empty fields:
> > 
> > ```
> > // Using tokenize makes these two equivalent.
> > [1-25-6]
> > [1-2,5-6]
> > ```
> > 
> > Can you add a test that we don't allow the extra commas since it's not 
> > the canonical format?
> > 
> > Also, why did you keep tokenizing on newlines? AFAICT they are not part 
> > of the canonical format?

Replaced tokenize with split, also add related test cases.


- Klaus


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/#review143028
---


On July 21, 2016, 10:38 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49223/
> ---
> 
> (Updated July 21, 2016, 10:38 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5739
> https://issues.apache.org/jira/browse/MESOS-5739
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Value parsing code to only accept the canonical formats.
> NOTES: This patch isn't doing a complete validation against 
> the canonical format yet: the set entries is not validated as
> conforming to the Values.Text format yet.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 
> 
> Diff: https://reviews.apache.org/r/49223/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/
---

(Updated July 21, 2016, 10:38 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Address Ben's comments.


Bugs: MESOS-5739
https://issues.apache.org/jira/browse/MESOS-5739


Repository: mesos


Description (updated)
---

Fixed Value parsing code to only accept the canonical formats.
NOTES: This patch isn't doing a complete validation against 
the canonical format yet: the set entries is not validated as
conforming to the Values.Text format yet.


Diffs (updated)
-

  src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
  src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
  src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 

Diff: https://reviews.apache.org/r/49223/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/#review143028
---



Sorry for the delay, mostly some bugs around use of tokenize rather than split, 
and some cleanup suggestions.

Be sure to update the two values.cpp files in the same way, based on the 
comments.


src/common/values.cpp (lines 577 - 583)


This can just be:

```
// Remove all spaces.
string temp = strings::replace(text, " ", "");
```

Feel free to simplify this in another patch.



src/common/values.cpp (lines 589 - 594)


Do we still need this? Seems to me that we want to do the following:

```
// Ranges. E.g. [1-2,4-5]
if (strings::startsWith(temp, "[")) {
  if (!strings::endsWith(temp, "]")) {
return Error("Missing a closing bracket");
  }

  ...
}

if (strings::startsWith(temp, "{")) {
  if (!strings::endsWith(temp, "}")) {
return Error("Missing a closing brace");
  }
  
  ..
}
```

Also, why is this looking at parentheses? I only see brackets (ranges) and 
braces (set) being used.



src/common/values.cpp (line 604)


Looks like you want a strings::split here? strings::tokenize will ignore 
empty fields:

```
// Using tokenize makes these two equivalent.
[1-25-6]
[1-2,5-6]
```

Can you add a test that we don't allow the extra commas since it's not the 
canonical format?

Also, why did you keep tokenizing on newlines? AFAICT they are not part of 
the canonical format?



src/common/values.cpp (lines 604 - 606)


You don't need the temporary variable?

```
foreach (const string& token, strings::split(temp, ",")) {
  ...
}
```



src/common/values.cpp (line 607)


Seems like we need strings::split here to avoid accepting ranges like 
`--1--2-`



src/common/values.cpp (line 609)


This message seems confusing. How about:

Invalid range '1 to 2'; expected format is 'begin-end'



src/common/values.cpp (lines 614 - 615)


We use `numify` now, mind updating this and removing the include?

Feel free to post a cleanup patch in front of this one.



src/common/values.cpp (line 635)


Ditto comments from Ranges here. (tokenize and newline)



src/common/values.cpp (lines 635 - 636)


You don't need the temporary variable?

```
foreach (const string& token, strings::split(temp, ",")) {
  ...
}
```



src/common/values.cpp (lines 637 - 638)


Please document in the description that this isn't a complete validation 
against the canonical format because we aren't validating text yet. The current 
summary of this commit seems to suggest that after applying this patch we'll 
**only** accept the canonical format (but that's not quite true).



src/common/values.cpp (line 649)


Ditto about numify here (but please do it in a separate patch in 
front of this one).



src/tests/values_tests.cpp (lines 88 - 91)


Can we also test the braces?

```
  // Only a single pair of braces are allowed.
  EXPECT_ERROR(parse("{a,b}{c,d}"));
  EXPECT_ERROR(parse("{a,b},{c,d}"));
  EXPECT_ERROR(parse("{a,b,{c,d}}"));
```

Since this isn't rejected just yet, please update the description to 
reflect that this patch isn't doing a complete validation against the canonical 
format. We can do the following to make it clear to the reader of the test that 
we don't fully validate yet:

```
  // TODO(klaus1982): Only a single pair of braces should be allowed,
  // however we do not validate the set entries as conforming to the
  // Values.Text format yet.
  //
  // EXPECT_ERROR(parse("{a,b}{c,d}"));
  // EXPECT_ERROR(parse("{a,b},{c,d}"));
  // EXPECT_ERROR(parse("{a,b,{c,d}}"));
```


- Benjamin Mahler


On July 16, 2016, 2:39 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49223/
> ---
> 
> (Updated July 16, 2016, 2:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/#review142483
---



Patch looks great!

Reviews applied: [49223]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 16, 2016, 2:39 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49223/
> ---
> 
> (Updated July 16, 2016, 2:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5739
> https://issues.apache.org/jira/browse/MESOS-5739
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Value parsing code to only accept the canonical formats.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 
> 
> Diff: https://reviews.apache.org/r/49223/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/
---

(Updated July 16, 2016, 10:39 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Update v1/value.cpp & add more cases


Bugs: MESOS-5739
https://issues.apache.org/jira/browse/MESOS-5739


Repository: mesos


Description (updated)
---

Fixed Value parsing code to only accept the canonical formats.


Diffs (updated)
-

  src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
  src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
  src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 

Diff: https://reviews.apache.org/r/49223/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/
---

(Updated July 9, 2016, 8:35 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Update the length of description.


Bugs: MESOS-5739
https://issues.apache.org/jira/browse/MESOS-5739


Repository: mesos


Description (updated)
---

Fixed Value parsing code to only accept the canonical formats defined
in: http://mesos.apache.org/documentation/latest/attributes-resources/


Diffs
-

  src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

Diff: https://reviews.apache.org/r/49223/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/#review141396
---



Your patch cannot be applied with description exceed 72 characters.

- Guangya Liu


On 七月 9, 2016, 7:54 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49223/
> ---
> 
> (Updated 七月 9, 2016, 7:54 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5739
> https://issues.apache.org/jira/browse/MESOS-5739
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Value parsing code to only accept the canonical formats defined in: 
> http://mesos.apache.org/documentation/latest/attributes-resources/
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/49223/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49223/
---

(Updated July 9, 2016, 3:54 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Updated summary based on BenM's comments.


Summary (updated)
-

Fixed Value parsing code to only accept the canonical formats.


Bugs: MESOS-5739
https://issues.apache.org/jira/browse/MESOS-5739


Repository: mesos


Description (updated)
---

Fixed Value parsing code to only accept the canonical formats defined in: 
http://mesos.apache.org/documentation/latest/attributes-resources/


Diffs
-

  src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

Diff: https://reviews.apache.org/r/49223/diff/


Testing
---

make && make check


Thanks,

Klaus Ma