You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2018/09/19 12:41:28 UTC
***UNCHECKED*** Re: Review Request 68756: Performed RP-specific
validations when adding/updating RP configs.
-----------------------------------------------------------
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
>
>
Re: Review Request 68756: Performed RP-specific validations when
adding/updating RP configs.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Sept. 19, 2018, 12:41 p.m., Benjamin Bannier wrote:
> > src/resource_provider/local.cpp
> > Lines 38-40 (patched)
> > <https://reviews.apache.org/r/68756/diff/1/?file=2090227#file2090227line39>
> >
> > 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;
> > ^~~~~~
> > ```
Oops. I meant to put `const` after `*`.
> On Sept. 19, 2018, 12:41 p.m., Benjamin Bannier wrote:
> > src/resource_provider/local.cpp
> > Lines 54-56 (patched)
> > <https://reviews.apache.org/r/68756/diff/1/?file=2090227#file2090227line55>
> >
> > 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`
Let me do an explicit list, as well as the `LocalResourceProviderCreatorBase` -> `ProviderAdaptor` renaming since it seems easier to maintain if we list the resource providers at one place instead of in 3 functions.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68756/#review208753
-----------------------------------------------------------
On Sept. 19, 2018, 4: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, 4: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
>
>