mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rojas <alexan...@mesosphere.io>
Subject Re: Review Request 58095: Refactored functions that render /roles and GetRoles endpoints.
Date Thu, 27 Apr 2017 14:58:03 GMT

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




src/master/http.cpp
Line 3401 (original), 3403 (patched)
<https://reviews.apache.org/r/58095/#comment246264>

    your changes here make this function non safe thread. Notice that the original would run
in the context of the `MasterProcess` thread, while this will do so in the caller's thread,
causing a possible race condition.



src/master/http.cpp
Lines 3481-3482 (original), 3456-3457 (patched)
<https://reviews.apache.org/r/58095/#comment246247>

    Not yours, but I feel the formatting of this lambda is really off. The body of the lambda
starts a couple of columns earlier than declaration.



src/master/http.cpp
Line 3483 (original), 3458-3459 (patched)
<https://reviews.apache.org/r/58095/#comment246258>

    I don't think it is a good idea to do this in a lambda. Did you consider follow the example
of the `FullFrameworkWriter`?
    
    Toying around I came with this abstraction:
    
    ```c++
    struct RoleWriter {
      RoleWriter(
          const Owned<ObjectApprover>& frameworksApprover,
          Master* master,
          const string& role)
        : frameworksApprover_(frameworksApprover), master_(master), role_(role)
      {
      }
    
      void operator()(JSON::ObjectWriter* writer)
      {
        writer->field("name", role_);
    
        writer->field(
            "weight",
            master_->weights.contains(role_) ? master->weights.at(role_) : 1.0);
    
        if (master_->quotas.contains(role_)) {
          writer->field("quota", master->quotas.at(role_));
        }
    
        if (master_->roles.contains(role_)) {
          Role* role = master_->roles.at(name);
          writer->field("resources", Resources(role->resources()));
          writer->field("frameworks", [this, role](JSON::ArrayWriter* writer) {
            foreachpair (
                const FrameworkID& frameworkId,
                const Framework* framework,
                role->frameworks) {
              if (!master_->approveViewFrameworkInfo(
                      frameworksApprover, framework->info)) {
                continue;
              }
              writer->element(frameworkId.value());
            }
          });
        } else {
          writer->field("resources", Resources());
          writer->field("frameworks", vector<int>());
        }
      }
    
      const Owned<ObjectApprover>& frameworksApprover_;
      Master* master_;
      const string& role_;
    };
    ```
    
    Note that the abstraction doesn't entirely work because of the visibility of Master, but
you can see how it reduces the ammount of nested lambdas leading to less indentantion layers
and improving readability.



src/master/http.cpp
Lines 3480-3490 (patched)
<https://reviews.apache.org/r/58095/#comment246259>

    Instead of completely serializing `QuotaInfo`, I suggest you to do one of two things,
first would be to replace this block for:
    
    ```c++
    writer->field("quota", JSON::Protobuf(quotaInfo));
    ```
    
    However, my prefered solution is the following function to `src/common/http.cpp`:
    
    ```c++
    // Need the namespace since the json override needs
    // to be in the same namespace as the type.
    namespace quota {
    void json(JSON::ObjectWriter* writer, const QuotaInfo& quotaInfo)
    {
      writer->field("guarantee", Resources(quotaInfo.guarantee()));
      writer->field("role", quotaInfo.role());
      if (quotaInfo.has_principal()) {
        writer->field("principal", quotaInfo.principal());
      }
    }
    } // namespace quota {
    ```
    
    then you can simply do:
    
    ```c++
    writer->field("quota", quotaInfo);
    ```



src/master/http.cpp
Line 3504 (original), 3505 (patched)
<https://reviews.apache.org/r/58095/#comment246261>

    Is ist really necesary to add empty fields?



src/master/http.cpp
Lines 3509 (patched)
<https://reviews.apache.org/r/58095/#comment246260>

    Serializing an empty vector will do the trick.



src/master/http.cpp
Line 3576 (original), 3594 (patched)
<https://reviews.apache.org/r/58095/#comment246263>

    It is not safe to use `[=]`, please change for a list of the things you actually want
to capture.



src/master/http.cpp
Line 3577 (original), 3595 (patched)
<https://reviews.apache.org/r/58095/#comment246262>

    No need to change that line, the returned `Response` object will be converted automatically
to a `Future<Response>`, plus it indicates that this lambda doesn't have any asynchronous
operations inside.
    
    On the other hand, if you fix the introduced race condition in `activeRoles()` this becomes
an asynchronous call.



src/master/master.hpp
Lines 1399-1401 (original), 1399-1405 (patched)
<https://reviews.apache.org/r/58095/#comment246246>

    How about rewording this to:
    
    ```c++
    // List of active roles, i.e. roles which are
    // explicitly white listed, plus roles with one or
    // more registered frameworks, plus all roles with a
    // non default weight or quota.
    ```
    
    This gives you a clear understanding of what the method returns without describing how
you do it (which is non interesting for API documentation). Also, the first paragraph doesn't
really says anything useful.


- Alexander Rojas


On April 19, 2017, 5:36 a.m., Jay Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> -----------------------------------------------------------
> 
> (Updated April 19, 2017, 5:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and Michael Park.
> 
> 
> Bugs: MESOS-4732 and MESOS-7260
>     https://issues.apache.org/jira/browse/MESOS-4732
>     https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Instead of generating JSON object, `Master::Http::roles()` now
> leverages `jsonify` to compute output. Also approver is taken
> out from its continuation function. This is for easier and cleaner
> implementation of framework authorization in /roles endpoint,
> see MESOS-7260.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
> 
> 
> Diff: https://reviews.apache.org/r/58095/diff/4/
> 
> 
> Testing
> -------
> 
> no functional changes
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>


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