You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/09/19 04:48:41 UTC
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/
-----------------------------------------------------------
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 in the next patch.
Thanks,
Chun-Hung Hsiao
Re: Review Request 68756: Performed RP-specific validations when
adding/updating RP configs.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68756/#review208841
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
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
>
>
Re: Review Request 68756: Performed RP-specific validations when
adding/updating RP configs.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68756/
-----------------------------------------------------------
(Updated Sept. 19, 2018, 6:36 p.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed Benjamin's comments.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/68756/diff/1-2/
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
>
>
***UNCHECKED*** Re: Review Request 68756: Performed RP-specific
validations when adding/updating RP configs.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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 Benjamin Bannier <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 68756: Performed RP-specific validations when
adding/updating RP configs.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68756/#review208759
-----------------------------------------------------------
src/resource_provider/local.hpp
Lines 47 (patched)
<https://reviews.apache.org/r/68756/#comment292904>
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.
- Jan Schlicht
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
>
>