> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 587 > > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line587> > > > > 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<vector<Resource>>`. > On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote: > > src/common/resources.cpp, line 645 > > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line645> > > > > 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 > > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line652> > > > > 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<vector<Resource>>` (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 > > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line697> > > > > 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 > > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line708> > > > > 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<vector<Resource>>` 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<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 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 > >
