You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2018/03/22 20:53:58 UTC
Review Request 66229: Implemented UPDATE_FRAMEWORK call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66229/
-----------------------------------------------------------
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
-----
include/mesos/scheduler/scheduler.proto 7a90355c3d1a362ebb4fdb63bfe1fba8aca60d5b
include/mesos/v1/scheduler/scheduler.proto f08fdfbab2a13d1104e7be4b42bf20e9048ab562
src/cli/execute.cpp 729896a4021a3be6dfd41a5f6bc78872170fe68a
src/examples/long_lived_framework.cpp 53e3ec99d4081a12e2138e79154b07ad5d5ef503
src/examples/test_csi_user_framework.cpp 91212e990af711633f7b890b0c9e10587f9efe7c
src/examples/test_http_framework.cpp 9ea3c6f35bf6464b4c6cfa7ba2eaf4febc168396
src/master/http.cpp 6f692e20b0f82dbe5c676739757b9eeaf4d6d49a
src/master/master.hpp 92af8520db15465fbe55c187be3945accc436ec3
src/master/master.cpp b06d7a6e2fbbb81b97eaf537d5b6745c73dc867d
src/master/metrics.hpp b343904bd99defffe098077855d686b2d33c1fc1
src/master/metrics.cpp b627372fd09b84aeda0b8e1b209d3bc0547faa39
src/master/validation.cpp b15b75c95bdffc6e62e84532f0457b439046562e
src/sched/sched.cpp 613a33b7d3150ad851af674e989a84b3899f2d69
src/tests/mesos.hpp f088cfdd490054ec605e33bd8c361a9d0939e22d
Diff: https://reviews.apache.org/r/66229/diff/1/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66229/#comment284093>
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-0000 (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)
<https://reviews.apache.org/r/66229/#comment284094>
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> error =
validation::scheduler::call::validate(call, master, principal)
```
src/master/metrics.cpp
Lines 265 (patched)
<https://reviews.apache.org/r/66229/#comment284108>
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.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66229/#comment284322>
Can you copy the TODO from above?
```
// TODO(anand): Throw java error.
```
src/master/http.cpp
Line 952 (original), 952-953 (patched)
<https://reviews.apache.org/r/66229/#comment284324>
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)
<https://reviews.apache.org/r/66229/#comment284330>
Let's `std::move()` it into the call here.
src/master/http.cpp
Lines 1135-1137 (patched)
<https://reviews.apache.org/r/66229/#comment284325>
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)
<https://reviews.apache.org/r/66229/#comment284326>
Does this still need to be public with the stateless validation?
src/master/master.cpp
Line 2346 (original), 2346 (patched)
<https://reviews.apache.org/r/66229/#comment284327>
Ditto here, let's keep it stateless at this point.
src/master/master.cpp
Lines 3407-3409 (original), 3411-3413 (patched)
<https://reviews.apache.org/r/66229/#comment284329>
"suppressed"
src/master/master.cpp
Lines 3441 (patched)
<https://reviews.apache.org/r/66229/#comment284332>
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)
<https://reviews.apache.org/r/66229/#comment284334>
Let's avoid the copy here, not sure why this code is copying..
src/master/master.cpp
Lines 3464-3466 (patched)
<https://reviews.apache.org/r/66229/#comment284333>
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)
<https://reviews.apache.org/r/66229/#comment284335>
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)
<https://reviews.apache.org/r/66229/#comment284336>
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)
<https://reviews.apache.org/r/66229/#comment284344>
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)
<https://reviews.apache.org/r/66229/#comment284339>
Is this still needed if we keep the call validation as stateless validation?
src/master/validation.cpp
Lines 508-509 (patched)
<https://reviews.apache.org/r/66229/#comment284343>
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)
<https://reviews.apache.org/r/66229/#comment284340>
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)
<https://reviews.apache.org/r/66229/#comment284341>
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)
<https://reviews.apache.org/r/66229/#comment284338>
Can you send a separate patch for this or just commit it directly on master?
src/master/validation.cpp
Lines 721 (patched)
<https://reviews.apache.org/r/66229/#comment284337>
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:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> -----------------------------------------------------------
>
> (Updated May 3, 2018, 4:05 a.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/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/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/12/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On April 12, 2018, 8:19 p.m., Till Toenshoff wrote:
> > src/master/validation.cpp
> > Lines 532 (patched)
> > <https://reviews.apache.org/r/66229/diff/4/?file=1996013#file1996013line532>
> >
> > 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.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66229/#comment281996>
Great, I can actually use this right away with some of my work as well - thanks!
src/master/master.cpp
Lines 3440 (patched)
<https://reviews.apache.org/r/66229/#comment282000>
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)
<https://reviews.apache.org/r/66229/#comment281998>
Leave the parameter name out for that one, just like the others.
src/master/master.cpp
Lines 3481 (patched)
<https://reviews.apache.org/r/66229/#comment282023>
If you used `*framework` instead of `frameworkInfo.name()`, the resulting logging could be even more helpful.
src/master/master.cpp
Lines 3490 (patched)
<https://reviews.apache.org/r/66229/#comment282021>
See above and remove the leading tick here.
src/master/validation.hpp
Lines 117 (patched)
<https://reviews.apache.org/r/66229/#comment282010>
s/Valides/Validates/
src/master/validation.cpp
Lines 509 (patched)
<https://reviews.apache.org/r/66229/#comment282038>
s/frameworkInfos/'FrameworkInfo's/ ?
src/master/validation.cpp
Lines 526 (patched)
<https://reviews.apache.org/r/66229/#comment282027>
s/newFrameworkInfo.name()/*framework/
src/master/validation.cpp
Lines 532 (patched)
<https://reviews.apache.org/r/66229/#comment282013>
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)
<https://reviews.apache.org/r/66229/#comment282014>
s/suppression/Suppression/
src/master/validation.cpp
Line 538 (original), 597 (patched)
<https://reviews.apache.org/r/66229/#comment282036>
Thanks, that's better.
src/master/validation.cpp
Lines 712 (patched)
<https://reviews.apache.org/r/66229/#comment282037>
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.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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