mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 51999: Refactor parsing of resources.
Date Mon, 03 Oct 2016 23:45:36 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message