Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Joris Van Remoortere]

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



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, 3:43 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated July 21, 2016, 3:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 
> 
> 
> Diff: https://reviews.apache.org/r/43561/diff/7/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50280, 50285, 49223, 43561]

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 21, 2016, 3:43 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated July 21, 2016, 3:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
>   src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
>   src/v1/values.cpp ff1926f9ab26c3ac3ab62b6df3ce305f5d12f12e 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

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

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


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
Remoortere.


Changes
---

Update depdencies.


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


Repository: mesos


Description
---

Improve Ranges parsing to handle single values.


Diffs (updated)
-

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

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


Testing
---

make
make check GTEST_FILTER=~"*"
./src/mesos-test


Thanks,

Klaus Ma

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Benjamin Mahler]

> On March 21, 2016, 2:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 613
> > 
> >
> > @benm I wish we had support for iterating over these splicers eg:
> > `foreachtoken(temp, ",\n", [](const string& token) { ... });`
> > 
> > These helpers become rather heavy (at 3 nested) considering their 
> > utility.
> 
> Klaus Ma wrote:
> @joris, I think `foreachtoken` is also nested loop. When doing the 
> patches, there're three options to me: FSM, regex and nested loop; FSM & 
> regex seems hard to maintain, so I used nested loop.
> 
> Klaus Ma wrote:
> `foreachtoken(temp, ",\n", [](const string& token) { ... });` is a good 
> helper function; but no performance improvement. Any comments?
> 
> Klaus Ma wrote:
> @joris, I logged MESOS-5234 to trace this; I'll update patches 
> accordingly.
> 
> Klaus Ma wrote:
> @joris, had a patch for `foreachtoken`; would you help to review it?
> 
> https://reviews.apache.org/r/46425/

Joris why not do this?

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

By the way, we're cleaning up this code in https://reviews.apache.org/r/49223/ 
and other patches. This patch should become trivial.


- Benjamin


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


On June 11, 2016, 2:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated June 11, 2016, 2:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Benjamin Mahler]

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



Can you rebase this on top of https://reviews.apache.org/r/49223/?

That should make this a much simpler patch, yes?

- Benjamin Mahler


On June 11, 2016, 2:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated June 11, 2016, 2:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, lines 612-627
> > 
> >
> > Should we add a benchmark here to understand the effect of now having 3 
> > levels of `tokenize`?
> > Is this code in any critical paths?
> 
> Klaus Ma wrote:
> This function is only uesd when parsing resources from string; it only 
> impacts the duration of `make check`. I'll try it and let you known the 
> impact.

Run `time build/src/mesos-test`, the result is almost the same :).


- Klaus


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


On June 11, 2016, 10:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated June 11, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On June 8, 2016, 5:32 a.m., Benjamin Mahler wrote:
> > src/tests/values_tests.cpp, lines 39-72
> > 
> >
> > It would be great to split apart this test into the respective value 
> > types, e.g.
> > 
> > ```
> > ValuesTest.ParseScalar
> > ValuesTest.ParseRanges
> > ValuesTest.ParseSet
> > ```
> > 
> > With more test cases that show what is currently accepted and what is 
> > not. There are a lot of other formats currently accepted from what I can 
> > tell (see my comment below).

Logged MESOS-5603 to trace those test refactor; I'll continue to work on this.

https://issues.apache.org/jira/browse/MESOS-5603


- Klaus


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


On June 11, 2016, 10:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated June 11, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43561]

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 June 11, 2016, 2:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated June 11, 2016, 2:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On June 8, 2016, 5:32 a.m., Benjamin Mahler wrote:
> > src/tests/values_tests.cpp, line 210
> > 
> >
> > If I understand correctly, the following are currently accepted way to 
> > specify 1-4:
> > 
> > `[[1-4]]`
> > `[1-2]\n[3-4]`
> > `[1-2],[3-4]`
> > 
> > I'm ok with rejecting these in favor of the following canonical formats:
> > 
> > `[1-2,3-4]`
> > 
> > Although it doesn't have to be coalesced. However, we should notify the 
> > user mailing list that we're moving towards more strict parsing of ranges 
> > if you're going to change what we accept.
> > 
> > Ideally, we could pull in a regex or parsing expression grammar library 
> > to more formally define our formats. The current parsing code is a mess.

Logged MESOS-5602 to trace "expression grammar library", 
https://issues.apache.org/jira/browse/MESOS-5602

And i'll send an email to dev@ and user@ list for this notification.


- Klaus


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


On June 11, 2016, 10:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated June 11, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

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

(Updated June 11, 2016, 10:29 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
Remoortere.


Changes
---

rebase


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


Repository: mesos


Description
---

Improve Ranges parsing to handle single values.


Diffs (updated)
-

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

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


Testing
---

make
make check GTEST_FILTER=~"*"
./src/mesos-test


Thanks,

Klaus Ma

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On June 8, 2016, 5:32 a.m., Benjamin Mahler wrote:
> > src/common/values.cpp, lines 654-657
> > 
> >
> > Can you pull this cleanup into a separate patch? Note that you don't 
> > need the variable:
> > 
> > ```
> >   foreach (const string& token, sstrings::tokenize(temp, "{},\n")) {
> > set->add_item(token);
> >   }
> > ```

Sorry for un-based code, this's handled in r/45813.


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On June 8, 2016, 5:32 a.m., Benjamin Mahler wrote:
> > Hi Klaus sorry for the delay.
> > 
> > This piece of code is very old in Mesos and has some significant technical 
> > debt. We should have used a parsing library here but at the time we didn't 
> > have  because we hadn't moved to C++11 yet. From what I can tell, 
> > regex needs GCC 4.9+ to work correctly in the std:: namespace. There 
> > appears to be a tr1 regex library. Can you do some investigation on whether 
> > we can pull in a regex library? I would suggest mailing the dev@ list to 
> > see if anyone has looked into this before.
> > 
> > Let me know what you find, I'm hesitant to make the parsing even more 
> > complicated and hacky here.
> > 
> > Until then, I would suggest splitting apart the tests here for each value 
> > type, and show all accepted formats (there are a lot that we'll accept 
> > currently that we don't show in the tests!).

Sure, I'll do some investigation on regex library :).


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Benjamin Mahler]

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



Hi Klaus sorry for the delay.

This piece of code is very old in Mesos and has some significant technical 
debt. We should have used a parsing library here but at the time we didn't have 
 because we hadn't moved to C++11 yet. From what I can tell, regex needs 
GCC 4.9+ to work correctly in the std:: namespace. There appears to be a tr1 
regex library. Can you do some investigation on whether we can pull in a regex 
library? I would suggest mailing the dev@ list to see if anyone has looked into 
this before.

Let me know what you find, I'm hesitant to make the parsing even more 
complicated and hacky here.

Until then, I would suggest splitting apart the tests here for each value type, 
and show all accepted formats (there are a lot that we'll accept currently that 
we don't show in the tests!).


src/common/values.cpp (lines 651 - 654)


Can you pull this cleanup into a separate patch? Note that you don't need 
the variable:

```
  foreach (const string& token, sstrings::tokenize(temp, "{},\n")) {
set->add_item(token);
  }
```



src/tests/values_tests.cpp (lines 39 - 72)


It would be great to split apart this test into the respective value types, 
e.g.

```
ValuesTest.ParseScalar
ValuesTest.ParseRanges
ValuesTest.ParseSet
```

With more test cases that show what is currently accepted and what is not. 
There are a lot of other formats currently accepted from what I can tell (see 
my comment below).



src/tests/values_tests.cpp (line 210)


If I understand correctly, the following are currently accepted way to 
specify 1-4:

`[[1-4]]`
`[1-2]\n[3-4]`
`[1-2],[3-4]`

I'm ok with rejecting these in favor of the following canonical formats:

`[1-2,3-4]`

Although it doesn't have to be coalesced. However, we should notify the 
user mailing list that we're moving towards more strict parsing of ranges if 
you're going to change what we accept.

Ideally, we could pull in a regex or parsing expression grammar library to 
more formally define our formats. The current parsing code is a mess.


- Benjamin Mahler


On March 22, 2016, 1:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 1:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 613
> > 
> >
> > @benm I wish we had support for iterating over these splicers eg:
> > `foreachtoken(temp, ",\n", [](const string& token) { ... });`
> > 
> > These helpers become rather heavy (at 3 nested) considering their 
> > utility.
> 
> Klaus Ma wrote:
> @joris, I think `foreachtoken` is also nested loop. When doing the 
> patches, there're three options to me: FSM, regex and nested loop; FSM & 
> regex seems hard to maintain, so I used nested loop.
> 
> Klaus Ma wrote:
> `foreachtoken(temp, ",\n", [](const string& token) { ... });` is a good 
> helper function; but no performance improvement. Any comments?
> 
> Klaus Ma wrote:
> @joris, I logged MESOS-5234 to trace this; I'll update patches 
> accordingly.

@joris, had a patch for `foreachtoken`; would you help to review it?

https://reviews.apache.org/r/46425/


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 613
> > 
> >
> > @benm I wish we had support for iterating over these splicers eg:
> > `foreachtoken(temp, ",\n", [](const string& token) { ... });`
> > 
> > These helpers become rather heavy (at 3 nested) considering their 
> > utility.
> 
> Klaus Ma wrote:
> @joris, I think `foreachtoken` is also nested loop. When doing the 
> patches, there're three options to me: FSM, regex and nested loop; FSM & 
> regex seems hard to maintain, so I used nested loop.
> 
> Klaus Ma wrote:
> `foreachtoken(temp, ",\n", [](const string& token) { ... });` is a good 
> helper function; but no performance improvement. Any comments?

@joris, I logged MESOS-5234 to trace this; I'll update patches accordingly.


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, lines 612-627
> > 
> >
> > Should we add a benchmark here to understand the effect of now having 3 
> > levels of `tokenize`?
> > Is this code in any critical paths?

This function is only uesd when parsing resources from string; it only impacts 
the duration of `make check`. I'll try it and let you known the impact.


> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 613
> > 
> >
> > @benm I wish we had support for iterating over these splicers eg:
> > `foreachtoken(temp, ",\n", [](const string& token) { ... });`
> > 
> > These helpers become rather heavy (at 3 nested) considering their 
> > utility.
> 
> Klaus Ma wrote:
> @joris, I think `foreachtoken` is also nested loop. When doing the 
> patches, there're three options to me: FSM, regex and nested loop; FSM & 
> regex seems hard to maintain, so I used nested loop.

`foreachtoken(temp, ",\n", [](const string& token) { ... });` is a good helper 
function; but no performance improvement. Any comments?


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 612
> > 
> >
> > Not yours: We shouldn't capture temporaries by reference.
> > Either:
> > 1) capture by value: `const vector tokens = ...`
> > 2) iterator over the result:
> > ```
> > foreach (const string& token, strings::tokenize(...)) {
> > ```
> > 
> > I mention it because you copy the pattern below.
> 
> Joris Van Remoortere wrote:
> There are a couple more of these that you missed. No need to update the 
> review again, i'll just re-open this and you can fix them after bmahler can 
> take a look.
> 
> Klaus Ma wrote:
> Updated patches for related code in this files :).

@joris, I logged a new RR to address this issue: 
https://reviews.apache.org/r/45813/. Would you help to review?


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 612
> > 
> >
> > Not yours: We shouldn't capture temporaries by reference.
> > Either:
> > 1) capture by value: `const vector tokens = ...`
> > 2) iterator over the result:
> > ```
> > foreach (const string& token, strings::tokenize(...)) {
> > ```
> > 
> > I mention it because you copy the pattern below.
> 
> Joris Van Remoortere wrote:
> There are a couple more of these that you missed. No need to update the 
> review again, i'll just re-open this and you can fix them after bmahler can 
> take a look.

Updated patches for related code in this files :).


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

> On March 21, 2016, 10:02 p.m., Joris Van Remoortere wrote:
> > src/common/values.cpp, line 613
> > 
> >
> > @benm I wish we had support for iterating over these splicers eg:
> > `foreachtoken(temp, ",\n", [](const string& token) { ... });`
> > 
> > These helpers become rather heavy (at 3 nested) considering their 
> > utility.

@joris, I think `foreachtoken` is also nested loop. When doing the patches, 
there're three options to me: FSM, regex and nested loop; FSM & regex seems 
hard to maintain, so I used nested loop.


- Klaus


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


On March 22, 2016, 9:22 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 22, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

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

(Updated March 22, 2016, 9:22 p.m.)


Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
Remoortere.


Changes
---

Address comments to avoid reference on tmp var.


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


Repository: mesos


Description
---

Improve Ranges parsing to handle single values.


Diffs (updated)
-

  src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
  src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing
---

make
make check GTEST_FILTER=~"*"
./src/mesos-test


Thanks,

Klaus Ma

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

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

(Updated March 22, 2016, 8:25 p.m.)


Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
Remoortere.


Changes
---

Address Joris' comments.


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


Repository: mesos


Description
---

Improve Ranges parsing to handle single values.


Diffs (updated)
-

  src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
  src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing
---

make
make check GTEST_FILTER=~"*"
./src/mesos-test


Thanks,

Klaus Ma

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Joris Van Remoortere]

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




src/common/values.cpp (line 612)


Not yours: We shouldn't capture temporaries by reference.
Either:
1) capture by value: `const vector tokens = ...`
2) iterator over the result:
```
foreach (const string& token, strings::tokenize(...)) {
```

I mention it because you copy the pattern below.



src/common/values.cpp (lines 612 - 626)


Should we add a benchmark here to understand the effect of now having 3 
levels of `tokenize`?
Is this code in any critical paths?



src/common/values.cpp (line 613)


@benm I wish we had support for iterating over these splicers eg:
`foreachtoken(temp, ",\n", [](const string& token) { ... });`

These helpers become rather heavy (at 3 nested) considering their utility.


- Joris Van Remoortere


On March 19, 2016, 10:08 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated March 19, 2016, 10:08 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp a545100522bf4b1f03e50656d461b3cda6b41e11 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43561]

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

- Mesos ReviewBot


On Feb. 29, 2016, 9:57 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated Feb. 29, 2016, 9:57 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
>   src/tests/resources_tests.cpp a545100522bf4b1f03e50656d461b3cda6b41e11 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

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

(Updated Feb. 29, 2016, 5:57 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebase


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


Repository: mesos


Description
---

Improve Ranges parsing to handle single values.


Diffs (updated)
-

  src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 
  src/tests/resources_tests.cpp a545100522bf4b1f03e50656d461b3cda6b41e11 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing
---

make
make check GTEST_FILTER=~"*"
./src/mesos-test


Thanks,

Klaus Ma

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43561]

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

- Mesos ReviewBot


On Feb. 14, 2016, 6:38 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43561/
> ---
> 
> (Updated Feb. 14, 2016, 6:38 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4627
> https://issues.apache.org/jira/browse/MESOS-4627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improve Ranges parsing to handle single values.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
>   src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/43561/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER=~"*"
> ./src/mesos-test
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43561: Improve Ranges parsing to handle single values.

[Klaus Ma]

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

(Updated Feb. 14, 2016, 2:38 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Enhance test cases.


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


Repository: mesos


Description
---

Improve Ranges parsing to handle single values.


Diffs (updated)
-

  src/common/values.cpp c64407bc97ad858300f4661d616e0480920fc541 
  src/tests/resources_tests.cpp 96864c3945729fe33be8b243b9c826fb12e90eff 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing (updated)
---

make
make check GTEST_FILTER=~"*"
./src/mesos-test


Thanks,

Klaus Ma