Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Committing with the following adjustment.


src/v1/resources.cpp (line 636)


Let's perserve this TODO as I think some form of this is still necessary.


- Jiang Yan Xu


On Oct. 5, 2016, 7 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 5, 2016, 7 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSON() to parse JSON representation of resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

> On Oct. 5, 2016, 11:06 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 167
> > 
> >
> > "If that fails" gave me the wrong idea of the condition under which the 
> > method would then call `fromSimpleString()` (so I suggested 
> > `fromJSONString(text, role)`).
> > 
> > 
> > We are basically just replying on the input type and not "failures" to 
> > choose the parsing mechanism so would the following be more clear?
> > 
> > 
> > Parses Resources from text in the form of a JSON array or as a simple 
> > string in the form of "name(role):value;name:value;...". i.e, this
> > method calls `fromJSONArray()` or `fromSimpleString()` and validates 
> > the resulting `vector` before converting it to a `Resources` 
> > object.
> > ```
> > 
> > The last part of the paragraph may be implied but I think it makes it 
> > more clear.

s/replying/relying/


- Jiang Yan


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


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Overall LGTM!


include/mesos/resources.hpp (line 157)


"If that fails" gave me the wrong idea of the condition under which the 
method would then call `fromSimpleString()` (so I suggested 
`fromJSONString(text, role)`).

We are basically just replying on the input type and not "failures" to 
choose the parsing mechanism so would the following be more clear?


Parses Resources from text in the form of a JSON array or as a simple 
string in the form of "name(role):value;name:value;...". i.e, this
method calls `fromJSONArray()` or `fromSimpleString()` and validates the 
resulting `vector` before converting it to a `Resources` object.
```

The last part of the paragraph may be implied but I think it makes it more 
clear.



include/mesos/resources.hpp (line 158)


s/i.e,/i.e.,/



include/mesos/resources.hpp (line 159)


s/fromJSONArray/fromJSON/ 

See the comment at the method declaration below.



include/mesos/resources.hpp (line 180)


Should we add a usage note?

```
NOTE: The `Resource` objects in the result vector may not be semantically 
valid (i.e., they may not pass `Resources::validate()`). This is to allow 
additional handling of the parsing results in certain cases.
```



include/mesos/resources.hpp (line 186)


s/fromJSONArray/fromJSON/

There should be no ambiguity and we gained brevity. 

The info about the argument being `JSON::Array` is already baked into the 
method signture so we don't have to say it again.



include/mesos/resources.hpp (line 197)


Should we add a usage note?

```
NOTE: The `Resource` objects in the result vector may not be semantically 
valid (i.e., they may not pass `Resources::validate()`). This is to allow 
additional handling of the parsing results in certain cases.
```



src/common/resources.cpp (lines 532 - 550)


Would the following be simpler?

```
Try _result = resourcesJSON.isSome()
  ? fromJSONArray(resourcesJSON.get(), defaultRole),
  : fromSimpleString(text, defaultRole);

if (_result.isError()) {
  return Error(_result.error());
}

resources = _result.get();
```

Also s/resourcesJSON/json/? (because it literally just checks whether it's 
valid JSON and it
is shorter)



src/common/resources.cpp (line 598)


```
// However, those checks are done at the respective call sites. 
```

I suggested above that we move this over to be a usage note for this 
method. If we do that then here the implementation really doesn't care about it 
one way or the other (as a lower level parsing function) so we can kill it.



src/common/resources.cpp (lines 646 - 647)


Ditto.


- Jiang Yan Xu


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All 

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 199
> > 
> >
> > s/JSON::Array& resourcesJSON/std::string text/
> > 
> > Looking the current and potential call-sites, it doesn't look like we 
> > ever directly call this method with a `JSON::Array` not parsed from text.
> > 
> > Considering this, I think it would be more consistent to have the 
> > following three methods which take the same argument list.
> > 
> > ```
> > static Try parse(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > 
> > static Try fromJSONString(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > 
> > static Try fromSimpleString(
> > const std::string& text,
> > const std::string& defaultRole = "*")
> > ```
> 
> Anindya Sinha wrote:
> For JSON handling, we do the following in sequence:
> (1) `JSON::parse()` converts input string to JSON array. 
> (2) `protobuf::parse(resourcesJSON)` converts 
> the JSON array to protobuf.
> 
> If we call both of these in `fromJSONString()`, then in 
> `Resources::parse()`, we would not be able to deduce which of the above 2 
> functions failed. This is needed since if it failed in (1), it means we parse 
> assuming text; whereas if it failed in (2), that is an error since protobuf 
> conversion failed for the JSON (we should not assume it is text).
> 
> So, I think this should work:
> In `Resources::parse()`:
> - Convert to JSON array (`JSON::parse()`).
> - If `JSON::parse()` succeeds, process JSON array in 
> `fromJSONString()`. Any failure here is an error condition.
> - If `JSON::parse()` fails, assume it is text and process in 
> `fromSimpleString()`.

OK, makes sense. Some of my other comments were based on the assumption of this 
method taking a string. I see that Guangya has already made comments about 
renaming it to `fromJSONArray()`. I'll add some additional comments.


> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, lines 630-657
> > 
> >
> > With out refactor, the result of `Resources::parse()` doesn't change 
> > right?
> 
> Jiang Yan Xu wrote:
> s/out/our/
> 
> Anindya Sinha wrote:
> Im original code: If a resource in text format is encountered with value 
> of 0, it is dropped; but in JSON, it flags an error. This patch makes the 
> behavior same, ie. just drop if a `Resource` has a value of 0. Hence, this 
> test would fail for value of 0. So I modified this test to test for negative 
> values which is obviously an Error.

OK, I don't think parse should return Error for empty values either.


- Jiang Yan


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


On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Oct. 3, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addresed review comments, and rebased.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 587
> > 
> >
> > I think it is not good to update here in this patch as I cannot see the 
> > reason why do we want to update here in this patch, can we move this update 
> > to the patch where it is needed?

I think the change is appropriate here since the calling functions now return 
`Try`.


> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 645
> > 
> >
> > As the API `fromJSONString(fromJSONArry as I proposed)` will also be 
> > called by agent to parse agent resource flags, so here may not

The `Resources::validate()` call is moved to `Resources::parse()` and also 
added in the private function in containerizer.cpp in 
https://reviews.apache.org/r/51879/ (since that patch is where we introduced 
auto detection if value is not specified). But I moved that change now in this 
patch to follow the chain easily.


> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 652
> > 
> >
> > As the `fromJSONString` (I prefer we rename this as fromJSONArray) will 
> > also be called in containerizer to parse resources from agent flag, so here 
> > we also need to call `validateCommandLineResources` to make sure there is 
> > no persistent volume, revocable resources etc.
> > 
> > Also if we add the `validateCommandLineResources` here we can remove it 
> > from #553 to #556.

I would keep the call to `internal::validateCommandLineResources()` from the 
top level function. The 2 functions `fromJSONArray()` and `fromSimpleString()` 
returns an unvalidated `Try` (also added that in the header 
of the respective function definition in resources.hpp), and the top level 
function needs to ensure it is valid in its own context by calling:
(1) `Resources::validate()` for generic validation of each `Resource` object.
(2) `internal::validateCommandLineResources()` to take care of persistent 
volumes, dynamic reservations and so on.

The top level functions are `Resources::parse()` as in this patch and from the 
private static function in containerizer.cpp (`parse()` which is added in 
https://reviews.apache.org/r/51879/ which handles parsing agent 
`flags.resources`.


> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 697-698
> > 
> >
> > I think we do not need validate here as here the `Resource` is get from 
> > `name, value, role`.

Agreed. See my previous response for why it was the way it is. However, I moved 
that change now in this patch to follow the chain easily.


> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 708
> > 
> >
> > Seems we still need `validateCommandLineResources` here as 
> > `fromSimpleString` will also be called in containerizer when parsing 
> > resources from agent flag, we should make sure there is no persistent 
> > volume, revocable etc.
> > 
> > After add `validateCommandLineResources` here, we can remove the 
> > validation from #553 to #556

As indicated before (and specified in the function signature in the header 
files), the functions `fromJSONArray()` and `fromSimpleString()` return an 
unvalidated `Try` which is validated by calling 
`Resources::validate()` and `internal::validateCommandLineResources()` from the 
top level functions, viz. `Resources::parse()` [in this patch], and from the 
private static function in containerizer.cpp (parse() which is added in 
https://reviews.apache.org/r/51879/.


- Anindya


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


On Sept. 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing 

Re: Review Request 51999: Refactor parsing of resources.

[Guangya Liu]

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




include/mesos/resources.hpp (line 172)


Not a `JSON string` but `JSON Array`



include/mesos/resources.hpp (line 174)


Not `from text` but from `JSON Array`



include/mesos/resources.hpp (line 181)


s/text/resourcesJSON



include/mesos/resources.hpp (line 186)


s/fromJSONString/fromJSONArray



src/common/resources.cpp (line 528)


I think it is not good to update here in this patch as I cannot see the 
reason why do we want to update here in this patch, can we move this update to 
the patch where it is needed?



src/common/resources.cpp (line 586)


As the API `fromJSONString(fromJSONArry as I proposed)` will also be called 
by agent to parse agent resource flags, so here may not



src/common/resources.cpp (line 593)


As the `fromJSONString` (I prefer we rename this as fromJSONArray) will 
also be called in containerizer to parse resources from agent flag, so here we 
also need to call `validateCommandLineResources` to make sure there is no 
persistent volume, revocable resources etc.

Also if we add the `validateCommandLineResources` here we can remove it 
from #553 to #556.



src/common/resources.cpp (lines 638 - 639)


I think we do not need validate here as here the `Resource` is get from 
`name, value, role`.



src/common/resources.cpp (line 646)


Seems we still need `validateCommandLineResources` here as 
`fromSimpleString` will also be called in containerizer when parsing resources 
from agent flag, we should make sure there is no persistent volume, revocable 
etc.

After add `validateCommandLineResources` here, we can remove the validation 
from #553 to #556


- Guangya Liu


On 九月 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

> On Sept. 27, 2016, 5:35 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 195-196
> > 
> >
> > We should probably add a note below
> > 
> > ```
> > The returned vector of Resource objects haven't gone through 
> > validations such as `Resources::validate(resource)`.
> > ```
> > 
> > Same for `fromSimpleString()`.

I added this in https://reviews.apache.org/r/51879/ since that is where the 
change regarding validation lies.


- Anindya


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


On Sept. 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Sept. 28, 2016, 7:24 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 478-484
> > 
> >
> > Per our discussion I thought we would delay the validation until when 
> > we construct `Resources` objects? (i.e., kill this).
> > 
> > The result is that the two more primitive parsing functions 
> > `fromJSONString()` and `fromSimpleString()` only captures syntactic errors 
> > (malformatted strings) but not semantic errors. We can then take advantage 
> > of this in the later patch to fill in missing values to make them 
> > semantically valid.

This was done in https://reviews.apache.org/r/51879/ when I added support for 
auto detection since that is when we could not longer do a `validate()` at this 
point.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 199
> > 
> >
> > s/JSON::Array& resourcesJSON/std::string text/
> > 
> > Looking the current and potential call-sites, it doesn't look like we 
> > ever directly call this method with a `JSON::Array` not parsed from text.
> > 
> > Considering this, I think it would be more consistent to have the 
> > following three methods which take the same argument list.
> > 
> > ```
> > static Try parse(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > 
> > static Try fromJSONString(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > 
> > static Try fromSimpleString(
> > const std::string& text,
> > const std::string& defaultRole = "*")
> > ```

For JSON handling, we do the following in sequence:
(1) `JSON::parse()` converts input string to JSON array. 
(2) `protobuf::parse(resourcesJSON)` converts the 
JSON array to protobuf.

If we call both of these in `fromJSONString()`, then in `Resources::parse()`, 
we would not be able to deduce which of the above 2 functions failed. This is 
needed since if it failed in (1), it means we parse assuming text; whereas if 
it failed in (2), that is an error since protobuf conversion failed for the 
JSON (we should not assume it is text).

So, I think this should work:
In `Resources::parse()`:
- Convert to JSON array (`JSON::parse()`).
- If `JSON::parse()` succeeds, process JSON array in 
`fromJSONString()`. Any failure here is an error condition.
- If `JSON::parse()` fails, assume it is text and process in 
`fromSimpleString()`.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 587-617
> > 
> >
> > Is the following cleaner? (I added the logic to validate resources in 
> > the result vector instead of implicit conversion)
> > 
> > ```
> > Try resources = fromJSONString(text, defaultRole);
> > 
> > // Parsing as a simple string if parsing as a JSON string fails.
> > if (resources.isError()) {
> >   resources = fromSimpleString(text, defaultRole);
> > }
> > 
> > if (resources.isError()) {
> >   return Error(resources.error());
> > }
> > 
> > Resources result;
> > 
> > foreach (const Resource& resource, resoruces) {
> >   // If invalid, propgate error instead of skipping the resource.
> >   Option error = Resources::validate(resource);
> >   
> >   if (error.isSome()) {
> > return error.get();
> >   }
> >   
> >   result += resource;
> > }
> > 
> > Option error = internal::validateCommandLineResources(result);
> > if (error.isSome()) {
> >   return error.get();
> > }
> > 
> > return result;
> > ```
> 
> Guangya Liu wrote:
> 1) Can we kill the validation? Seems we already done the validation in 
> each sub function.
> 
> ```
> // If invalid, propgate error instead of skipping the resource.
> Option error = Resources::validate(resource);
> ```
> 
> 2) Using `result.add(resource)` instead.

@xujyan: Please refer to my earlier response as to why we need to convert to 
JSON in `Resources::parse()` and handle the JSON in `fromJSONString()` if JSON 
conversion is successful [If conversion to protobuf fails in 
`fromJSONString()`, that is an error]; and assume it is text and process in 
`fromSimpleString()` if JSON conversion fails.

@gyliu: 
(1) This patch still has the validation in individual functions so validation 
in `Resources::parse()` is not needed but in  
https://reviews.apache.org/r/51879/, we move the validation out of the 
individual functions and consolidate into `Resources::parse()` [since we can 
have `Resource` with no value].
(2) Using `result.add()` now.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > 

Re: Review Request 51999: Refactor parsing of resources.

[Guangya Liu]

> On 九月 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 587-617
> > 
> >
> > Is the following cleaner? (I added the logic to validate resources in 
> > the result vector instead of implicit conversion)
> > 
> > ```
> > Try resources = fromJSONString(text, defaultRole);
> > 
> > // Parsing as a simple string if parsing as a JSON string fails.
> > if (resources.isError()) {
> >   resources = fromSimpleString(text, defaultRole);
> > }
> > 
> > if (resources.isError()) {
> >   return Error(resources.error());
> > }
> > 
> > Resources result;
> > 
> > foreach (const Resource& resource, resoruces) {
> >   // If invalid, propgate error instead of skipping the resource.
> >   Option error = Resources::validate(resource);
> >   
> >   if (error.isSome()) {
> > return error.get();
> >   }
> >   
> >   result += resource;
> > }
> > 
> > Option error = internal::validateCommandLineResources(result);
> > if (error.isSome()) {
> >   return error.get();
> > }
> > 
> > return result;
> > ```

1) Can we kill the validation? Seems we already done the validation in each sub 
function.

```
// If invalid, propgate error instead of skipping the resource.
Option error = Resources::validate(resource);
```

2) Using `result.add(resource)` instead.


- Guangya


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


On 九月 26, 2016, 8:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 26, 2016, 8:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Guangya Liu]

> On 九月 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 677
> > 
> >
> > I think that we should still use 
> > `internal::validateCommandLineResources` as the command line resources 
> > should not have dynamic resevations etc.
> > 
> > But there is a bug in `internal::validateCommandLineResources` now, as 
> > when I set a resource string as `cpus:45.55;ports:[1-2, 
> > 3-5];disks:{-xx}`, there will be no erros if using 
> > `internal::validateCommandLineResources` to validate.
> > 
> > So I would suggest the following:
> > 1) Continue use `internal::validateCommandLineResources` to validate 
> > resource.
> > 2) Add `role` validation in `internal::validateCommandLineResources`.
> 
> Anindya Sinha wrote:
> I am not sure I understand your concern. In `fromJsonString()` [actually 
> `convertJson()`] and `fromSimpleString()`, we call `Resources::validate()` to 
> validate each of the `Resource` objects (which captures negative valued 
> resource, etc. as an Error). In addition, we call 
> `internal::validateCommandLineResources` from `Resources::parse()` after 
> getting the `vector` from `fromJsonString()` and 
> `fromSimpleString()` to validate again the resource containing persistent 
> volume, dynamic reservation, etc. So, I think we are covered on your case.
> 
> Secondly, can you please explain what you mean by 'role' validation from 
> point # 2 above.

1) I think we cannot call `Resources::validate()` in `fromSimpleString()`, as 
the input is from flags text and we cannot specify reservation etc, so we need 
to make sure there is no reservation etc after `fromSimpleString()`. But in 
`fromSimpleString()`, you are actually calling `Resources::validate()` which 
will think that having reservation etc is also valid, that's why I propose call 
`internal::validateCommandLineResources` instead. 

2) Another point I want to mention is when calling `fromSimpleString()` and 
using `internal::validateCommandLineResources`, we should also validate `roles` 
as here we are also setting roles for the resource object, so we also need to 
add role validation logic, otherwise, there will be no error when parsing 
`cpus:45.55;disks:{-xx}`, (NOTE: `disks:{-xx}` is not valid)


- Guangya


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


On 九月 26, 2016, 8:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 26, 2016, 8:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

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




include/mesos/resources.hpp (lines 191 - 192)


We should probably add a note below

```
The returned vector of Resource objects haven't gone through validations 
such as `Resources::validate(resource)`.
```

Same for `fromSimpleString()`.


- Jiang Yan Xu


On Sept. 26, 2016, 1:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 26, 2016, 1:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

> On Sept. 27, 2016, 10:28 a.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, lines 630-657
> > 
> >
> > With out refactor, the result of `Resources::parse()` doesn't change 
> > right?

s/out/our/


- Jiang Yan


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


On Sept. 26, 2016, 1:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 26, 2016, 1:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

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




include/mesos/resources.hpp (lines 56 - 64)


Per my reply to a previous comment, can we kill this?



include/mesos/resources.hpp (line 165)


The JSON string is still text and the argument is called `text`.

Can we keep the original summary?



include/mesos/resources.hpp 


Now that we've moved these words to avoid duplication, we'd better refer to 
the place this is moved to.

```
  /**
   * Parses Resources from text in the form of a JSON array. If that fails, 
   * text in the form "name(role):value;name:value;...". i.e, this method
   * calls `fromJSONString()` or `fromSimpleString()` (see their 
documentation
   * for for details) and validates the result before converting it into a 
   * Resources object.
   */
```



include/mesos/resources.hpp (line 187)


Not yours, but s/cpus"/"cpus"/



include/mesos/resources.hpp (line 195)


s/JSON::Array& resourcesJSON/std::string text/

Looking the current and potential call-sites, it doesn't look like we ever 
directly call this method with a `JSON::Array` not parsed from text.

Considering this, I think it would be more consistent to have the following 
three methods which take the same argument list.

```
static Try parse(
const std::string& text,
const std::string& defaultRole = "*");

static Try fromJSONString(
const std::string& text,
const std::string& defaultRole = "*");

static Try fromSimpleString(
const std::string& text,
const std::string& defaultRole = "*")
```



include/mesos/resources.hpp (lines 522 - 524)


No need for this.



src/common/resources.cpp (lines 434 - 450)


Kill the method and use the contents to implement `fromJSONString`



src/common/resources.cpp (lines 456 - 457)


Not yours, but 2 space indentation, could you fix it now that we are 
refactoring?



src/common/resources.cpp (line 461)


Not yours but wrap operators at the end of the line for consistency.



src/common/resources.cpp (lines 473 - 477)


Per our discussion I thought we would delay the validation until when we 
construct `Resources` objects? (i.e., kill this).

The result is that the two more primitive parsing functions 
`fromJSONString()` and `fromSimpleString()` only captures syntactic errors 
(malformatted strings) but not semantic errors. We can then take advantage of 
this in the later patch to fill in missing values to make them semantically 
valid.



src/common/resources.cpp (lines 580 - 610)


Is the following cleaner? (I added the logic to validate resources in the 
result vector instead of implicit conversion)

```
Try resources = fromJSONString(text, defaultRole);

// Parsing as a simple string if parsing as a JSON string fails.
if (resources.isError()) {
  resources = fromSimpleString(text, defaultRole);
}

if (resources.isError()) {
  return Error(resources.error());
}

Resources result;

foreach (const Resource& resource, resoruces) {
  // If invalid, propgate error instead of skipping the resource.
  Option error = Resources::validate(resource);
  
  if (error.isSome()) {
return error.get();
  }
  
  result += resource;
}

Option error = internal::validateCommandLineResources(result);
if (error.isSome()) {
  return error.get();
}

return result;
```



src/common/resources.cpp (lines 618 - 625)


This method itself pratically does nothing in this form. We can simply move 
the contents of `convertJSON` over.



src/common/resources.cpp (lines 669 - 673)


Same as `fromJSONString()`, kill this.



src/tests/resources_tests.cpp (lines 630 - 657)


With out refactor, the result of `Resources::parse()` doesn't change right?


- Jiang Yan Xu


On Sept. 26, 2016, 1:58 p.m., Anindya Sinha wrote:
> 
> 

Re: Review Request 51999: Refactor parsing of resources.

[Jiang Yan Xu]

> On Sept. 20, 2016, 10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 60-64
> > 
> >
> > We can get rid of this forward declaration if we can get rid of the 
> > internal convertJSON.
> 
> Anindya Sinha wrote:
> Based on the refactor, we are keeping `convertJSON()`.

Why are we keeping `convertJSON()`?

`convertJSON` was orginally a private helper, now we are making it public 
because it's useful externally. Now that it's made public, keeping a separate 
`convertJSON()` with a single caller `fromJSONString()` feels redundant. It's 
basically the same thing except that `convertJSON()`, as a friend of Resources, 
can access the private `add()` method but now `fromJSONString/convertJSON` 
doesn't even rely on it anymore (it returns a vector). Even the need for access 
to `add()` in `Resources::parse(text, defaultRole)` is not obvious due to where 
it is typically called from.

As a result, can we kill it?


- Jiang Yan


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


On Sept. 26, 2016, 1:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 26, 2016, 1:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Sept. 26, 2016, 8:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Sept. 26, 2016, 6:46 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated based on review comments.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 183-217
> > 
> >
> > I think we do not intend to expose those two APIs, so `private` them?

We would call these two APIs from `Containerizer()` to parse `flags.resources`. 
So we cannot `private` them.


> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 677
> > 
> >
> > I think that we should still use 
> > `internal::validateCommandLineResources` as the command line resources 
> > should not have dynamic resevations etc.
> > 
> > But there is a bug in `internal::validateCommandLineResources` now, as 
> > when I set a resource string as `cpus:45.55;ports:[1-2, 
> > 3-5];disks:{-xx}`, there will be no erros if using 
> > `internal::validateCommandLineResources` to validate.
> > 
> > So I would suggest the following:
> > 1) Continue use `internal::validateCommandLineResources` to validate 
> > resource.
> > 2) Add `role` validation in `internal::validateCommandLineResources`.

I am not sure I understand your concern. In `fromJsonString()` [actually 
`convertJson()`] and `fromSimpleString()`, we call `Resources::validate()` to 
validate each of the `Resource` objects (which captures negative valued 
resource, etc. as an Error). In addition, we call 
`internal::validateCommandLineResources` from `Resources::parse()` after 
getting the `vector` from `fromJsonString()` and `fromSimpleString()` 
to validate again the resource containing persistent volume, dynamic 
reservation, etc. So, I think we are covered on your case.

Secondly, can you please explain what you mean by 'role' validation from point 
# 2 above.


> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 200
> > 
> >
> > This was only called by `Try parse` and the `Try 
> > parse` already defined the default value as `defaultRole` as star role, so 
> > we do not need to set the parameter `defaultRole` default to star role. But 
> > adding a default value may make the code more easy to understand.

Although I do not see the need to do so, I can add the default `defaultRole` 
for clarity.


> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 587
> > 
> >
> > Why not 
> > 
> > ```
> > Resources resources;
> > ```

For this patch, I agreee that using a `Resourecs` object here would be fine. 
However, if you look at https://reviews.apache.org/r/51879/ (which is in this 
chain), we would need this to be a `vector`. So I would not modify it 
here.


- Anindya


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


On Sept. 21, 2016, 4:14 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 21, 2016, 4:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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




include/mesos/resources.hpp (lines 179 - 213)


We would need to call these APIs from `Containerizer` for parsing of 
`flags.resources` so I do not think we can `private` these APIs.



src/common/resources.cpp (line 670)


I am not sure I understand your concern. In `fromJsonString()` [actually 
`convertJson()`] and `fromSimpleString()`, individual `Resource` object is 
validated. In `Resources::parse()`, we call 
`internal::validateCommandLineResources(resources)` to check for dynamic 
reservation, persistent volume, etc.

Secondly, what are you implying by `role` validation? Would you please 
elaborate?


- Anindya Sinha


On Sept. 26, 2016, 6:46 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 26, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Guangya Liu]

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




include/mesos/resources.hpp (lines 179 - 213)


I think we do not intend to expose those two APIs, so `private` them?



include/mesos/resources.hpp (line 196)


This was only called by `Try parse` and the `Try 
parse` already defined the default value as `defaultRole` as star role, so we 
do not need to set the parameter `defaultRole` default to star role. But adding 
a default value may make the code more easy to understand.



include/mesos/resources.hpp (line 213)


ditto



src/common/resources.cpp (line 580)


Why not 

```
Resources resources;
```



src/common/resources.cpp (line 670)


I think that we should still use `internal::validateCommandLineResources` 
as the command line resources should not have dynamic resevations etc.

But there is a bug in `internal::validateCommandLineResources` now, as when 
I set a resource string as `cpus:45.55;ports:[1-2, 
3-5];disks:{-xx}`, there will be no erros if using 
`internal::validateCommandLineResources` to validate.

So I would suggest the following:
1) Continue use `internal::validateCommandLineResources` to validate 
resource.
2) Add `role` validation in `internal::validateCommandLineResources`.


- Guangya Liu


On 九月 21, 2016, 4:14 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 21, 2016, 4:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 187-188
> > 
> >
> > Why update here, I prefer
> > 
> > ```
> > parses text in the form "name(role):value;name:value;...".
> > ```

I do not think it matters. The change was done since the previous line did not 
fit this within 80 chars.
Anyways with the refactor, this is not an issue anymore.


> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 201
> > 
> >
> > I think that we should find a better name for this, the above API 
> > `parse` is actually also parsing a text.
> > 
> > What about `parseResourceVector` or else some other meaningful name in 
> > your mind?

Resolved with the refactor.


> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, line 64
> > 
> >
> > I suggest that we move this to 
> > https://reviews.apache.org/r/51879/diff/3#index_header

Resolved with the refactor.


- Anindya


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


On Sept. 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Sept. 21, 2016, 4:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated based on review comments.


Summary (updated)
-

Refactor parsing of resources.


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


Repository: mesos


Description (updated)
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing (updated)
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

> On Sept. 20, 2016, 5 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 60-64
> > 
> >
> > We can get rid of this forward declaration if we can get rid of the 
> > internal convertJSON.

Based on the refactor, we are keeping `convertJSON()`.


> On Sept. 20, 2016, 5 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 201
> > 
> >
> > This is too similar to 
> > 
> > ```
> > static Try parse(
> >   const std::string& text,
> >   const std::string& defaultRole = "*");
> > ```
> > 
> > I was originally suggesting moving over convertJSON but I guess it's 
> > more suitable here in the following form:
> > 
> > ```
> > Try fromJSONString(
> > const std::string& text,
> > const std::string& defaultRole)
> > ```
> > 
> > Similarly for simple flat strings:
> > 
> > ```
> > Try fromSimpleString(
> > const std::string& text,
> > const std::string& defaultRole)
> > ```
> > 
> > When we have both, then 
> > 
> > ```
> > static Try parse(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > ```
> > 
> > becomes a mere convenience method that calls the above two methods in 
> > sequence. The convenience method can be simply documented as:
> > 
> > ```
> >   /** 
> >* Returns Resources from an input string.
> >*
> >* Parses Resources from text in the form of a JSON array (see
> >* `fromJSONString()`). If that fails, parses text in the simple 
> >* string form (see `fromSimpleString()`).
> >*/
> > ```
> > 
> > How does it sound?

Well, we can do this in a couple of ways. The current approach was to use 
`Resources::parseText()` [or we could call it `Resources::parse()` with an 
additional parameter to include or not include empty resources], which 
`Resources::parse()` would call to implicitly convert to `Resources` from 
`vecor`, whereas in `Containerizer::resources()`, we would call 
`Resources::parseText()` or `Resources::parse()` with the additional parameter.
Another option we discussed is to expose another `Resources::parse()` that 
returns `vecor` in a separate namespace which probably is cleaner but 
would require refactor in other call sites.
For now, we would follow the approach specified above.


- Anindya


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


On Sept. 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Jiang Yan Xu]

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




include/mesos/resources.hpp (lines 60 - 64)


We can get rid of this forward declaration if we can get rid of the 
internal convertJSON.



include/mesos/resources.hpp (line 183)


Use `/**`.



include/mesos/resources.hpp (line 201)


This is too similar to 

```
static Try parse(
  const std::string& text,
  const std::string& defaultRole = "*");
```

I was originally suggesting moving over convertJSON but I guess it's more 
suitable here in the following form:

```
Try fromJSONString(
const std::string& text,
const std::string& defaultRole)
```

Similarly for simple flat strings:

```
Try fromSimpleString(
const std::string& text,
const std::string& defaultRole)
```

When we have both, then 

```
static Try parse(
const std::string& text,
const std::string& defaultRole = "*");
```

becomes a mere convenience method that calls the above two methods in 
sequence. The convenience method can be simply documented as:

```
  /** 
   * Returns Resources from an input string.
   *
   * Parses Resources from text in the form of a JSON array (see
   * `fromJSONString()`). If that fails, parses text in the simple 
   * string form (see `fromSimpleString()`).
   */
```

How does it sound?


- Jiang Yan Xu


On Sept. 19, 2016, 3:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 3:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Guangya Liu]

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



Ditto for v1


include/mesos/resources.hpp (lines 187 - 188)


Why update here, I prefer

```
parses text in the form "name(role):value;name:value;...".
```



include/mesos/resources.hpp (line 201)


I think that we should find a better name for this, the above API `parse` 
is actually also parsing a text.

What about `parseResourceVector` or else some other meaningful name in your 
mind?



src/common/resources.cpp (line 586)


s/vector to Resources/`vector` to `Resources` object



src/slave/containerizer/containerizer.cpp (line 64)


I suggest that we move this to 
https://reviews.apache.org/r/51879/diff/3#index_header


- Guangya Liu


On 九月 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Guangya Liu]

> On 九月 19, 2016, 6 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 64-71
> > 
> >
> > A question here: Does the `resources` here will include the zero value 
> > resource?
> > 
> > If so, what is the differencen of this code block and 
> > `Resources::parse()`? The `Resources::parse()` seems similar as here.
> 
> Anindya Sinha wrote:
> The original code returns a `Resources` which contains individual 
> `Resource` objects, which removes any empty `Resource` objects 
> (https://github.com/apache/mesos/blob/master/src/common/resources.cpp#L1610).
> Since we need to know of empty `Resource` objects for this patch, we 
> actually accumulate the `Resource` objects in a `vector` (which 
> will contain any empty `Resource` object) which is used when disk resources 
> are present (to check for scalar value of 0 for the auto detection logic). 
> But for the rest of the resources, we do a `Resources resources = 
> parsed.get();` which implicitly converts a `vector` to `Resources` 
> which removes the empty `Resource` objects.

I see, that's why this update makes me confused: You are actually doing the 
same thing as `Resources::parse()` here.

I think that it is not good to put the update here but merge this to 
https://reviews.apache.org/r/51879/diff/3#index_header , comments?


- Guangya


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


On 九月 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 10:42 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


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


Repository: mesos


Description
---

During parsing of resources in the startup flags of mesos agent, all
the valid resources (including empty resources) are accumulated in a
vector of Resource protobufs.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

> On Sept. 19, 2016, 6 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 64-71
> > 
> >
> > A question here: Does the `resources` here will include the zero value 
> > resource?
> > 
> > If so, what is the differencen of this code block and 
> > `Resources::parse()`? The `Resources::parse()` seems similar as here.

The original code returns a `Resources` which contains individual `Resource` 
objects, which removes any empty `Resource` objects 
(https://github.com/apache/mesos/blob/master/src/common/resources.cpp#L1610).
Since we need to know of empty `Resource` objects for this patch, we actually 
accumulate the `Resource` objects in a `vector` (which will contain 
any empty `Resource` object) which is used when disk resources are present (to 
check for scalar value of 0 for the auto detection logic). But for the rest of 
the resources, we do a `Resources resources = parsed.get();` which implicitly 
converts a `vector` to `Resources` which removes the empty `Resource` 
objects.


> On Sept. 19, 2016, 6 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 200
> > 
> >
> > A unit test is needed for this.

Tests for `Containerizer::resources()` -> `Resources::parseText()` are added in 
https://reviews.apache.org/r/51880/


- Anindya


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


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Guangya Liu]

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




include/mesos/resources.hpp (line 184)


s/Resource/`Resource` objects

ditto for v1



include/mesos/resources.hpp (line 186)


s/Resource/`Resource` objects

ditto for v1



include/mesos/resources.hpp (line 197)


s/Resources/vector of `Resource` objects

ditto for v1



include/mesos/resources.hpp (line 200)


A unit test is needed for this.



src/common/resources.cpp (line 435)


s/Resource protobufs/`Resource` objects



src/common/resources.cpp (line 437)


s/Resources object/vector of `Resource` objects



src/common/resources.cpp (line 441)


kill `and empty Resource objects will return an error.`



src/common/resources.cpp (line 447)


s/Resource protobufs/`Resource` objects



src/common/resources.cpp (line 580)


s/Try result/const Try result



src/slave/containerizer/containerizer.cpp (lines 64 - 71)


A question here: Does the `resources` here will include the zero value 
resource?

If so, what is the differencen of this code block and `Resources::parse()`? 
The `Resources::parse()` seems similar as here.


- Guangya Liu


On 九月 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Guangya Liu]

> On 九月 19, 2016, 6 a.m., Guangya Liu wrote:
> >

Ditto for all v1 files.


- Guangya


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


On 九月 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 5:01 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Add reviewer.


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


Repository: mesos


Description
---

During parsing of resources in the startup flags of mesos agent, all
the valid resources (including empty resources) are accumulated in a
vector of Resource protobufs.


Diffs
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

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

Review request for mesos.


Repository: mesos


Description
---

During parsing of resources in the startup flags of mesos agent, all
the valid resources (including empty resources) are accumulated in a
vector of Resource protobufs.


Diffs
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha