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