> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote: > > include/mesos/resources.hpp, lines 183-217 > > <https://reviews.apache.org/r/51999/diff/3/?file=1506809#file1506809line183> > > > > 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 > > <https://reviews.apache.org/r/51999/diff/3/?file=1506811#file1506811line677> > > > > 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:[10000-20000, > > 30000-50000];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<Resource>` 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 > > <https://reviews.apache.org/r/51999/diff/3/?file=1506809#file1506809line200> > > > > This was only called by `Try<Resources> parse` and the `Try<Resources> > > 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 > > <https://reviews.apache.org/r/51999/diff/3/?file=1506811#file1506811line587> > > > > 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<Resource>`. 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<vector<Resource>>`, 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 > >
