mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 68756: Performed RP-specific validations when adding/updating RP configs.
Date Fri, 21 Sep 2018 09:23:17 GMT


> On Sept. 19, 2018, 2:43 p.m., Jan Schlicht wrote:
> > src/resource_provider/local.hpp
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/68756/diff/1/?file=2090226#file2090226line47>
> >
> >     This should be defined in `resource_provider/validation.hpp`, not here. Implementation
in `resource_provider/validation.cpp`.
> >     
> >     I do understand why this is done here and it's good to have a single code path
to dispatch from a resource provider type to its implementation. It would be great if we could
separate this from the abstract factory that `LocalResourceProvider` provides instead of dispatching
to static functions. But implementing a proper solution for this would add a lot of complexity
(e.g. have the abstract factory create an instance of a factory that can create `LocalResourceProvider`
implementation but also validate infos and probably do more in the future), so I'm okay with
using static dispatch here. Though we should think of this again if there will be more disparch
functions like this in the future.

I believe there is value in keeping it here. This is not really like the validation functions
we usually put into `validation.hpp` header files -- this here is a validation of a generic
`ResourceProviderInfo` proto container in the context of a concrete `Provider` making use
of it, not a validation of a proto container itself.


- Benjamin


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


On Sept. 19, 2018, 8:36 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68756/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2018, 8:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9228
>     https://issues.apache.org/jira/browse/MESOS-9228
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Each type of RP might have some specific validations for RP info. For
> example, SLRP requires the `storage` field to be set. This patch makes
> the local RP daemon to perform such validations when adding/updating
> configs, so the `ADD_RESOURCE_PROVIDER_CONFIG` and
> `UPDATE_RESOURCE_PROVIDER_CONFIG` calls can fail fast.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/daemon.cpp 0a76cc6d1d34413674d1af1aa514679a4d2b7b55 
>   src/resource_provider/local.hpp 20bcc78d3fe847e03526fa59116bdbac92ec1e29 
>   src/resource_provider/local.cpp 801e6c430ed91315d87f8a45b8f3ed128beca4fc 
>   src/resource_provider/storage/provider.hpp 5a371b19289c6e39fedd4cda65fa8be432d095e6

>   src/resource_provider/storage/provider.cpp 6475f653263337c381b6080695d09c49e5ea8fcf

>   src/slave/http.cpp f8199af50cf0b43bbbb8c29afe751fcd8949d7e8 
> 
> 
> Diff: https://reviews.apache.org/r/68756/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> New tests added later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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