mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 60013: Resources: Updated the validation logic.
Date Tue, 13 Jun 2017 19:19:04 GMT


> On June 13, 2017, 10:58 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 846-849 (original), 870-914 (patched)
> > <https://reviews.apache.org/r/60013/diff/1/?file=1748842#file1748842line870>
> >
> >     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.

Okay, the optimization is there because I'm already worried about this code because `Resources::validate`
as far as remember is one of the hottest paths.


- Michael


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


On June 13, 2017, 1: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, 1: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