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 60013: Resources: Updated the validation logic.
Date Tue, 13 Jun 2017 17:58:44 GMT

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




src/common/resources.cpp
Lines 847-848 (patched)
<https://reviews.apache.org/r/60013/#comment251415>

    We try not to use backticks in error or logging messages, they were introduced to provide
markup for comments. Can you replace them with single quotes here and elsewhere?



src/common/resources.cpp
Lines 854 (patched)
<https://reviews.apache.org/r/60013/#comment251416>

    Whoops, no periods in error messages



src/common/resources.cpp
Lines 844-845 (original), 864-868 (patched)
<https://reviews.apache.org/r/60013/#comment251476>

    Do you want to add a function to roles.hpp for checking this?
    
    ```
    bool isSubRole(const string& parentRole, const string& subRole);
    ```



src/common/resources.cpp
Lines 846-849 (original), 870-914 (patched)
<https://reviews.apache.org/r/60013/#comment251421>

    This was a little tough to read through, I wonder if the following would be easier to
read:
    
    ```
    // Validate all of the roles.
    foreach (const ReservationInfo& reservation, resource.reservations()) {
      Option<Error> error = roles::validate(reservation.role());
      if (error.isSome()) {
        return error;
      }
    
      if (reservation.role() == "*") {
        return Error("Invalid reservation: role \"*\" cannot be reserved");
      }
    }
    
    // Validate that each reservation is a refinement of the
    // preceding reservation.
    string parentRole = resource.reservations(0).role();
    
    for (int i = 1; i < resource.reservations_size(); ++i) {
      if (resource.reservations(i).type() ==
          Resource::ReservationInfo::STATIC) {
        return Error(
            "Invalid refined reservation: A refined reservation"
            " cannot be static.");
      }
    
      const string& childRole = resource.reservations(i).role();
      
      if (!isSubRole(parentRole, childRole)) {
        return Error(
            "Invalid refined reservation: role '" + childRole +
            "' is not a refinement of '" + parentRole + "'");
      }
      
      parentRole = role;
    }
    ```
    
    Granted, this doesn't have your tokenize optimization, but perhaps for now we just add
a TODO to optimize this? It seems to me there are bigger optimizations than just halving the
number of tokenizations, like avoiding the copying that tokenization incurs altogether.



src/common/resources.cpp
Lines 907-908 (patched)
<https://reviews.apache.org/r/60013/#comment251417>

    We try to open and close the quote on the same line (otherwise we tend to forget to close
because it's not as obvious), can you do that here?



src/master/validation.hpp
Lines 139 (patched)
<https://reviews.apache.org/r/60013/#comment251406>

    Option bool seems a little confusing to me, since I assume false is equivalent to None()?
    
    Why not just take optional framework info or to be more specific, optional framework capabilities
here?



src/master/validation.hpp
Line 154 (original), 155 (patched)
<https://reviews.apache.org/r/60013/#comment251407>

    Why not take optional framework capabilities or framework info here? Rather than the framework
state?



src/master/validation.hpp
Line 182 (original), 183 (patched)
<https://reviews.apache.org/r/60013/#comment251408>

    Ditto here



src/master/validation.hpp
Lines 188 (patched)
<https://reviews.apache.org/r/60013/#comment251409>

    Ditto here.



src/master/validation.cpp
Lines 601-603 (patched)
<https://reviews.apache.org/r/60013/#comment251413>

    Can you add a comment about what this is doing? i.e. that we have an old and new format
for resource role / reservations? Perhaps name this `validateReservationFormat` or something
that is a little more specific towards that?



src/master/validation.cpp
Lines 603 (patched)
<https://reviews.apache.org/r/60013/#comment251412>

    Why not just take the capabilities here, instead of taking the bool?



src/master/validation.cpp
Lines 609-610 (patched)
<https://reviews.apache.org/r/60013/#comment251410>

    I don't think we're using periods in these messages (we generally don't)
    
    Also, no backticks in error or logging messages (just comments) (some leaked in)
    
    Also, we have been generally trying to put the space on the next line, e.g. from below:
    
    ```
    return Error(
        "Dynamically reserved resource " + stringify(resource) +
        " cannot be created from revocable resources");
    ```



src/master/validation.cpp
Lines 615-616 (patched)
<https://reviews.apache.org/r/60013/#comment251411>

    Ditto here, no period / backticks.
    
    Also could you put the space on the next line to match our other logging? (We started
doing this because at the end of the line it's less obvious when we missed it).



src/master/validation.cpp
Line 1207 (original), 1258 (patched)
<https://reviews.apache.org/r/60013/#comment251414>

    We only passed the framework for validateUniqueTaskID because we needed to introspect
the state (i.e. tasks), whereas here we really just need the FrameworkInfo or capabilities?


- Benjamin Mahler


On June 13, 2017, 8:34 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60013/
> -----------------------------------------------------------
> 
> (Updated June 13, 2017, 8:34 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
>     https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Resources: Updated the validation logic.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
>   src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
>   src/tests/master_validation_tests.cpp 4e7ce74edf0069b9317f869b299694a45e2a62f2 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60013/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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