mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject ***UNCHECKED*** Re: Review Request 68756: Performed RP-specific validations when adding/updating RP configs.
Date Wed, 19 Sep 2018 12:41:28 GMT

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




src/resource_provider/daemon.cpp
Lines 463-464 (original), 463-464 (patched)
<https://reviews.apache.org/r/68756/#comment292902>

    Are we sure this is actually validated? The interface is flexible enough so that in the
future we might propagate `Error` states up to here. I'd suggest to either change the `create`
functions to always return some proper state, or leave the existing validation in. I'd lean
towards the latter.
    
    In any case, Let's `move` here since `Owned` is not semantically copyable.



src/resource_provider/local.cpp
Lines 38-40 (patched)
<https://reviews.apache.org/r/68756/#comment292898>

    These trigger warnings with clang,
    ```
    ../src/resource_provider/local.cpp:38:3: warning: 'const' qualifier on function type 'decltype(LocalResourceProvider::create)'
(aka 'Try<process::Owned<LocalResourceProvider> > (const process::http::URL &,
const basic_string<char> &, const mesos::ResourceProviderInfo &, const mesos::SlaveID
&, const Option<basic_string<char> > &, bool)') has no effect [-Wignored-qualifiers]
      const decltype(LocalResourceProvider::create)* create;
      ^~~~~~
    ../src/resource_provider/local.cpp:39:3: warning: 'const' qualifier on function type 'decltype(LocalResourceProvider::principal)'
(aka 'Try<process::http::authentication::Principal> (const mesos::ResourceProviderInfo
&)') has no effect [-Wignored-qualifiers]
      const decltype(LocalResourceProvider::principal)* principal;
      ^~~~~~
    ../src/resource_provider/local.cpp:40:3: warning: 'const' qualifier on function type 'decltype(LocalResourceProvider::validate)'
(aka 'Option<Error> (const mesos::ResourceProviderInfo &)') has no effect [-Wignored-qualifiers]
      const decltype(LocalResourceProvider::validate)* validate;
      ^~~~~~
    ```



src/resource_provider/local.cpp
Lines 54-56 (patched)
<https://reviews.apache.org/r/68756/#comment292903>

    This trait puts pretty strong requirements on the possible `TResourceProvider`s which
makes the code less flexible and doesn't buy us that much ATM.
    
    I'd prefer if we'd stay with the existing explicit references to the only concrete provider
functions we have now until we have another provider; we can always work toward modularization
later.
    
    If we'd decide to stay with this implementation we should at least do some renames, e.g.,
    * `LocalResourceProviderCreatorBase` -> `ProviderAdaptor`
    * `LocalResourceProviderCreator` -> `ProviderAdapatorTraits`



src/resource_provider/local.cpp
Lines 60-61 (patched)
<https://reviews.apache.org/r/68756/#comment292905>

    Not sure that this comment adds much or could even be enforced here. Suggest to drop.


- Benjamin Bannier


On Sept. 19, 2018, 6:48 a.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, 6:48 a.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/1/
> 
> 
> 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