Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-05-09 Thread Kapil Arya 

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

(Updated May 9, 2018, 9:15 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


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


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
ea8d54ff198a5529d61a41bcb6e5806378761091 
  src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad 
  src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 
  src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66229/diff/13/

Changes: https://reviews.apache.org/r/66229/diff/12-13/


Testing
---


Thanks,

Kapil Arya

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-05-06 Thread Benjamin Mahler 

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




src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 688 (patched)


Can you copy the TODO from above?

```
// TODO(anand): Throw java error.
```



src/master/http.cpp
Line 952 (original), 952-953 (patched)


Per discussion, let's keep this as stateless validation, and add the master 
level validation into the master's handler.



src/master/http.cpp
Lines 1133 (patched)


Let's `std::move()` it into the call here.



src/master/http.cpp
Lines 1135-1137 (patched)


Did authorization already happen at this point? If something isn't 
authorized, e.g. subscribing to one of the new roles, should this return 401?



src/master/master.hpp
Lines 584 (patched)


Does this still need to be public with the stateless validation?



src/master/master.cpp
Line 2346 (original), 2346 (patched)


Ditto here, let's keep it stateless at this point.



src/master/master.cpp
Lines 3407-3409 (original), 3411-3413 (patched)


"suppressed"



src/master/master.cpp
Lines 3441 (patched)


We're trying to update all of these to take rvalue references as part of 
the copy elimination work. You won't be able to move it into the .then lambda 
without C++14. If you add an `_updateFramework` then we could `std::move()` the 
call into the continuation. We should probably just do that now since it's 
going to come up during benchmarking.



src/master/master.cpp
Lines 3450 (patched)


Let's avoid the copy here, not sure why this code is copying..



src/master/master.cpp
Lines 3464-3466 (patched)


Can you avoid copying the roles into the set here? This gets expensive if 
frameworks have e.g. 10,000 roles.



src/master/master.cpp
Lines 3468 (patched)


Ideally the two parameters could be std::moved in. Let's not worry about it 
here. If you want to send a follow up patch to eliminate copies in this 
updateFramework() overload that would be great! If not, it's going to come up 
during benchmarking :)



src/master/metrics.hpp
Lines 142 (patched)


I don't think we want to make up v0 message metrics for v1 calls.

Can you ask greg/gaston about this? I think the plan is to try to map v0 to 
v1 calls instead of adding v0 style metrics for v1 calls.



src/master/validation.cpp
Lines 491-493 (patched)


The contents of this look familiar, is it copied from the re-registration 
path? Can you add appropriate TODOs about the inconsistency between the 
re-registration path and the UPDATE_FRAMEWORK path?



src/master/validation.cpp
Lines 495-499 (patched)


Is this still needed if we keep the call validation as stateless validation?



src/master/validation.cpp
Lines 508-509 (patched)


Just as an aside, this copying could get rather expensive for frameworks 
with e.g. 10,000 roles. Don't need to address it here, but the fix I suppose 
would be to do fine grained equality checking here.



src/master/validation.cpp
Lines 511-517 (patched)


I guess this function needs a significant update once it accepts the same 
changes to the framework info as are allowed in the re-registration path?



src/master/validation.cpp
Lines 535-536 (patched)


It's odd to log this one case, why isn't the caller logging this when 
logging the error?



src/master/validation.cpp
Line 538 (original), 606 (patched)


Can you send a separate patch for this or just commit it directly on master?



src/master/validation.cpp
Lines 721 (patched)


This function is just stateless validation for now, per above comments.

Ideally this function would also do stateless validation of the messages 
within the call, but that's a cleanup to do separately.


- Benjamin Mahler


On May 3, 2018, 4:05 a.m., Kapil Arya wrote:
> 
> ---
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-05-02 Thread Kapil Arya 

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

(Updated May 3, 2018, 12:05 a.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Changes
---

Addressed Till's comments.


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


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad 
  src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 
  src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66229/diff/9/

Changes: https://reviews.apache.org/r/66229/diff/8-9/


Testing
---


Thanks,

Kapil Arya

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-05-02 Thread Till Toenshoff 

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



As discussed in our direct review, some more changes on the 
future-continuations are needed - left those out here.


src/master/validation.cpp
Line 526 (original), 526 (patched)


Sorry I wasn't clear ... 

When streaming a `Framework` into a log output, we commonly do not use 
surrounding ticks or alike.

See e.g. 
https://github.com/apache/mesos/blob/868ec364b17bc231697563f2621705baff2d7183/src/master/master.cpp#L3075

Here is the implementation of that operator; 
https://github.com/apache/mesos/blob/868ec364b17bc231697563f2621705baff2d7183/src/master/master.hpp#L2965-L2978

And this is how that commonly looks in the end;
```
I0503 01:55:54.630450 236896256 master.cpp:3200] Framework 
a0035e5a-14eb-4f6d-873e-e3cea144b682- (default) at 
scheduler-ae407694-95bb-4188-90a9-07e55025c788@192.168.178.20:59261 failed over
```



src/master/http.cpp
Line 952 (original), 952-953 (patched)


We commonly do not use multiple arguments in a single line on an argument 
continuation -- rather unfortunate btw cause that is what mesos-clang-format 
does in some cases. So if we would format like you suggest, then we would 
actually have to break every parameter into a new line.

In this specific case breaking at the assignment, which is always 
preferred, actually solves it much nicer.

```
Option error =
  validation::scheduler::call::validate(call, master, principal)
```



src/master/metrics.cpp
Lines 265 (patched)


Thanks for this.


- Till Toenshoff


On April 20, 2018, 8:19 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> ---
> 
> (Updated April 20, 2018, 8:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad 
>   src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 
>   src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66229/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-04-20 Thread Kapil Arya 

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

(Updated April 20, 2018, 4:19 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


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


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
  src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
  src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66229/diff/6/

Changes: https://reviews.apache.org/r/66229/diff/5-6/


Testing
---


Thanks,

Kapil Arya

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-04-12 Thread Kapil Arya 


> On April 12, 2018, 8:19 p.m., Till Toenshoff wrote:
> > src/master/validation.cpp
> > Lines 532 (patched)
> > 
> >
> > s/is/are/
> > 
> > How do we make sure the role is "valid" - isn't that implicit when it 
> > was contained in the framework roles?

Updated the comment to reflect that we are validating "suppressed" roles.


- Kapil


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


On April 12, 2018, 9:38 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> ---
> 
> (Updated April 12, 2018, 9:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66229/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-04-12 Thread Kapil Arya 

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

(Updated April 12, 2018, 9:38 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Changes
---

Addressed Till's comments.


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


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
  src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
  src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66229/diff/5/

Changes: https://reviews.apache.org/r/66229/diff/4-5/


Testing
---


Thanks,

Kapil Arya

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-04-12 Thread Till Toenshoff 

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



Looks great Kapil - just some minor comments on logging and style.


src/master/master.hpp
Lines 584 (patched)


Great, I can actually use this right away with some of my work as well - 
thanks!



src/master/master.cpp
Lines 3440 (patched)


Which overload are we clashing with here? Or what is the reasoning?

Do you really need the `Master` namespace reference here?



src/master/master.cpp
Lines 3441 (patched)


Leave the parameter name out for that one, just like the others.



src/master/master.cpp
Lines 3481 (patched)


If you used `*framework` instead of `frameworkInfo.name()`, the resulting 
logging could be even more helpful.



src/master/master.cpp
Lines 3490 (patched)


See above and remove the leading tick here.



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


s/Valides/Validates/



src/master/validation.cpp
Lines 509 (patched)


s/frameworkInfos/'FrameworkInfo's/ ?



src/master/validation.cpp
Lines 526 (patched)


s/newFrameworkInfo.name()/*framework/



src/master/validation.cpp
Lines 532 (patched)


s/is/are/

How do we make sure the role is "valid" - isn't that implicit when it was 
contained in the framework roles?



src/master/validation.cpp
Lines 538 (patched)


s/suppression/Suppression/



src/master/validation.cpp
Line 538 (original), 597 (patched)


Thanks, that's better.



src/master/validation.cpp
Lines 712 (patched)


I really like this new, early specific validation.


- Till Toenshoff


On April 11, 2018, 5:28 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> ---
> 
> (Updated April 11, 2018, 5:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
>   src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66229/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

2018-04-11 Thread Kapil Arya 

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

(Updated April 11, 2018, 1:28 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


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


Repository: mesos


Description
---

Implemented UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
  src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 
  src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb 
  src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66229/diff/4/

Changes: https://reviews.apache.org/r/66229/diff/3-4/


Testing
---


Thanks,

Kapil Arya