Re: Review Request 60013: Resources: Updated the validation logic.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/common/resources.cpp
Lines 511-512 (original), 511-512 (patched)


Can you commit this unrelated formatting change separately? Would be good 
to minimize the diff for posterity



src/common/resources.cpp
Lines 522-523 (original), 522-523 (patched)


Ditto here



src/master/validation.cpp
Lines 1854-1856 (original), 1902-1904 (patched)


Commit this separately to minimize the diff?



src/master/validation.cpp
Lines 2027-2030 (original), 2091-2094 (patched)


Commit this separately to minimize the diff?



src/tests/master_validation_tests.cpp
Lines 1987-1989 (original), 1987 (patched)


This seems strictly less readable to me, the intention of capturing the 
'error' was to have a clear expectation:

```
EXPECT_NONE(error);
```

Ditto on the others below. It's also unrelated to this change :)



src/v1/resources.cpp
Lines 513-525 (original), 513-525 (patched)


Ditto v0 comments. Could you commit this separately in the interest of 
getting this diff down to its essential?


- Benjamin Mahler


On June 15, 2017, 7:39 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60013/
> ---
> 
> (Updated June 15, 2017, 7:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates `Resources::validate`, and `resource::validate`.
> 
> The `Resources::validate` function ensures that the resources are
> either in the 'pre-' or 'post-' reservation-refinement formats,
> but not both.
> 
> The `resource::validate` function takes an optional framework
> capabilities and ensures that the resources format matches
> the framework's RESERVATION_REFINEMENT capbility.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp e37519ea83c564b72c842c662fe784731c6e44b0 
>   src/master/validation.hpp 1be9f082024a5295e6a20bb047c15217c1866ca3 
>   src/master/validation.cpp 1d10e6bb0dfada7d6aa7085aa99c5bfa8099643e 
>   src/tests/master_validation_tests.cpp 
> 83370fa5653fe5da666ec577b05001c4a5499848 
>   src/v1/resources.cpp 40ef32f11be3f2710a0e634f3225314fb9ae5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60013/diff/13/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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

(Updated June 15, 2017, 12:39 a.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655


Repository: mesos


Description (updated)
---

This patch updates `Resources::validate`, and `resource::validate`.

The `Resources::validate` function ensures that the resources are
either in the 'pre-' or 'post-' reservation-refinement formats,
but not both.

The `resource::validate` function takes an optional framework
capabilities and ensures that the resources format matches
the framework's RESERVATION_REFINEMENT capbility.


Diffs (updated)
-

  src/common/resources.cpp 7b3fae1a34150964ba3831a3aef2f868e338aef8 
  src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
  src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
  src/tests/master_validation_tests.cpp 
4e7ce74edf0069b9317f869b299694a45e2a62f2 
  src/v1/resources.cpp 236fe17730918479a33314e51d14f3cc1679d432 


Diff: https://reviews.apache.org/r/60013/diff/11/

Changes: https://reviews.apache.org/r/60013/diff/10-11/


Testing
---


Thanks,

Michael Park

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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

(Updated June 14, 2017, 8:20 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed rest of bmahler's comments.


Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655


Repository: mesos


Description
---

Resources: Updated the validation logic.


Diffs (updated)
-

  src/common/resources.cpp 7b3fae1a34150964ba3831a3aef2f868e338aef8 
  src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
  src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
  src/tests/master_validation_tests.cpp 
4e7ce74edf0069b9317f869b299694a45e2a62f2 
  src/v1/resources.cpp 236fe17730918479a33314e51d14f3cc1679d432 


Diff: https://reviews.apache.org/r/60013/diff/8/

Changes: https://reviews.apache.org/r/60013/diff/7-8/


Testing
---


Thanks,

Michael Park

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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

(Updated June 14, 2017, 7:37 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed the comment around the complex loop.


Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655


Repository: mesos


Description
---

Resources: Updated the validation logic.


Diffs (updated)
-

  src/common/resources.cpp 7b3fae1a34150964ba3831a3aef2f868e338aef8 
  src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
  src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
  src/tests/master_validation_tests.cpp 
4e7ce74edf0069b9317f869b299694a45e2a62f2 
  src/v1/resources.cpp 236fe17730918479a33314e51d14f3cc1679d432 


Diff: https://reviews.apache.org/r/60013/diff/7/

Changes: https://reviews.apache.org/r/60013/diff/6-7/


Testing
---


Thanks,

Michael Park

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

> On June 13, 2017, 10:58 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 844-845 (original), 864-868 (patched)
> > 
> >
> > Do you want to add a function to roles.hpp for checking this?
> > 
> > ```
> > bool isSubRole(const string& parentRole, const string& subRole);
> > ```

https://reviews.apache.org/r/60063


- 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 7b3fae1a34150964ba3831a3aef2f868e338aef8 
>   src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
>   src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
>   src/tests/master_validation_tests.cpp 
> 4e7ce74edf0069b9317f869b299694a45e2a62f2 
>   src/v1/resources.cpp 236fe17730918479a33314e51d14f3cc1679d432 
> 
> 
> Diff: https://reviews.apache.org/r/60013/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

> On June 13, 2017, 10:58 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 846-849 (original), 870-914 (patched)
> > 
> >
> > 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 = 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
> 
>

Re: Review Request 60013: Resources: Updated the validation logic.

[Benjamin Mahler]

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


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)


Whoops, no periods in error messages



src/common/resources.cpp
Lines 844-845 (original), 864-868 (patched)


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)


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


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)


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)


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



src/master/validation.hpp
Line 182 (original), 183 (patched)


Ditto here



src/master/validation.hpp
Lines 188 (patched)


Ditto here.



src/master/validation.cpp
Lines 601-603 (patched)


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)


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



src/master/validation.cpp
Lines 609-610 (patched)


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)


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

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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


Changes
---

Rebased.


Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655


Repository: mesos


Description (updated)
---

Resources: Updated the validation logic.


Diffs (updated)
-

  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/

Changes: https://reviews.apache.org/r/60013/diff/1-2/


Testing
---


Thanks,

Michael Park

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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

(Updated June 12, 2017, 11:27 a.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655


Repository: mesos


Description (updated)
---

With reservation refinement, there is a new framework capability called 
`RESERVATION_REFINEMENT`. The framework is required to use the 
`Resource.reservations` field to express reservations if the capability is set, 
otherwise it is required to use the `Resource.role` and `Resource.reservation` 
fields.


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


Testing
---


Thanks,

Michael Park

Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7655
https://issues.apache.org/jira/browse/MESOS-7655


Repository: mesos


Description
---

See summary.


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


Testing
---


Thanks,

Michael Park