mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 71859: Moved creating authorization Object out of `Master::authorizeTask`.
Date Wed, 04 Dec 2019 18:21:56 GMT

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



I'm really excited to see the synchronous authorization improvement! Thanks for tackling this

Overall looks good, just a couple of copy related issues, and some style related notes.


src/common/authorization.cpp
Lines 58 (patched)
<https://reviews.apache.org/r/71859/#comment306816>

    Don't forget those braces on the next line! :)



src/common/authorization.cpp
Lines 59 (patched)
<https://reviews.apache.org/r/71859/#comment306817>

    can just be one line?
    
    ```
    return stream << jsonify(JSON::Protobuf(object));
    ```



src/master/authorization.hpp
Lines 32 (patched)
<https://reviews.apache.org/r/71859/#comment306819>

    Ditto here, brace on next line



src/master/authorization.hpp
Lines 36 (patched)
<https://reviews.apache.org/r/71859/#comment306818>

    s/:/: /
    
    Also, the following looks better?
    
    ```
      ActionObject(Action action_) : action(std::move(action_)) {};
    ```
    
    But I assume action is an enum? In that case, maybe we don't take it by ref or move it
either?



src/master/authorization.cpp
Lines 41-42 (patched)
<https://reviews.apache.org/r/71859/#comment306820>

    We try in new code to stick to the following assignment style, since it works automatically
with moves and is also easier to read:
    
    ```
      *actionObject.object->mutable_task_info() = task;
      *actionObject.object->mutable_framework_info() = framework;
    ```



src/master/authorization.cpp
Lines 44 (patched)
<https://reviews.apache.org/r/71859/#comment306821>

    Can you avoid the copy of the action object here?
    
    I'm guessing in the subsequent patches it will be more obvious why we return a vector
here (e.g. other action object creators require return multiple values and we want a uniform
interface?)



src/master/master.cpp
Line 3733 (original), 3740 (patched)
<https://reviews.apache.org/r/71859/#comment306825>

    Ditto here:
    
    ```
    *request.mutable_subject() = *subject;
    ```



src/master/master.cpp
Lines 3736-3741 (original), 3743-3751 (patched)
<https://reviews.apache.org/r/71859/#comment306826>

    Hm.. can you reply to this comment with an example of the logging before and after? I'm
curious if it's looking worse or we have less information (e.g. task id).



src/master/master.cpp
Lines 3755 (patched)
<https://reviews.apache.org/r/71859/#comment306827>

    thanks!



src/master/master.cpp
Lines 3759 (patched)
<https://reviews.apache.org/r/71859/#comment306828>

    why the "if one is available"? That seems to imply there still being an async fallback?



src/master/master.cpp
Lines 3743-3748 (original), 3763-3767 (patched)
<https://reviews.apache.org/r/71859/#comment306829>

    Why is this optimization up here rather than in collectAuthorizations?



src/master/master.cpp
Lines 4365-4377 (patched)
<https://reviews.apache.org/r/71859/#comment306822>

    This patch has introduced an extra copy of all the tasks! Notice that before the lambda
just returned a reference to the right field, now since it's not returning a reference we're
copying it.
    
    Can you leave it as it was as a lambda without the copying? I guess since we need to call
it twice now we move the declaration up:
    
    ```
            const RepeatedPtrField<TaskInfo>& tasks = [&]() {
              if (operation.type() == Offer::Operation::LAUNCH) {
                return operation.launch().task_infos();
              } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) {
                return operation.launch_group().task_group().tasks();
              }
              UNREACHABLE();
            }();
            
            // Use `tasks` twice below.. 
    ```



src/master/master.cpp
Lines 4825-4827 (original), 4843-4846 (patched)
<https://reviews.apache.org/r/71859/#comment306823>

    Why the change here? Can you leave the old wrapping style as is?



src/master/master.cpp
Lines 4851-4853 (patched)
<https://reviews.apache.org/r/71859/#comment306824>

    We tend to avoid wrapping way over to the right, so the following would be acceptable:
    
    ```
      const Option<Principal> principal = framework->info.has_principal()
        ? Principal(framework->info.principal())
        : Option<Principal>::none();
    ```
    
    ```
      const Option<Principal> principal = framework->info.has_principal()
        ? Principal(framework->info.principal()) : Option<Principal>::none();
    ```
    
    ```
      const Option<Principal> principal =
        framework->info.has_principal()
          ? Principal(framework->info.principal()) 
          : Option<Principal>::none();
    ```


- Benjamin Mahler


On Dec. 3, 2019, 3:02 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71859/
> -----------------------------------------------------------
> 
> (Updated Dec. 3, 2019, 3:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10023 and MESOS-10056
>     https://issues.apache.org/jira/browse/MESOS-10023
>     https://issues.apache.org/jira/browse/MESOS-10056
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This is the first patch in the chain that extract Master code
> generating Action-Object pairs into a dedicated ActionObject class,
> thus seperating authz Object creation from feeding them into authorizer.
> 
> This is a prerequisite to using ObjectApprover interface for
> synchronous authorization of Scheduler API calls.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt ef9382dc77d37fed344b7267119f3251acd3088a 
>   src/Makefile.am 111c156c8a7abc5ece04779e8ac8879a30c22dbf 
>   src/common/authorization.hpp 565d5ca6620442803fa80be652ab7382102347f5 
>   src/common/authorization.cpp fa71b0e8e8b9541376a9fd199f4d7b9db56a3f0f 
>   src/master/authorization.hpp PRE-CREATION 
>   src/master/authorization.cpp PRE-CREATION 
>   src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665 
>   src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 
> 
> 
> Diff: https://reviews.apache.org/r/71859/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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