You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2016/04/01 03:42:37 UTC
Review Request 45572: Add labels to ExecutorInfo and deprecate source.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-5029
https://issues.apache.org/jira/browse/MESOS-5029
Repository: mesos
Description
-------
Add labels to ExecutorInfo and deprecate source.
Diffs (updated)
-----
CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
Diff: https://reviews.apache.org/r/45572/diff/
Testing (updated)
-------
Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
Thanks,
Zhitao Li
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review128503
-----------------------------------------------------------
Fix it, then Ship it!
Thanks for the tests, I only commented on the master test since the same comments apply to the slave test.
Will get these cleaned up for you and will commit shortly.
CHANGELOG (line 43)
<https://reviews.apache.org/r/45572/#comment191913>
s/not/now/ ?
CHANGELOG (lines 56 - 58)
<https://reviews.apache.org/r/45572/#comment191923>
Since we mention the deprecation above, we probably don't need to mention it again here.
CHANGELOG (line 57)
<https://reviews.apache.org/r/45572/#comment191915>
ExecutorInfo typo
src/common/http.cpp (line 359)
<https://reviews.apache.org/r/45572/#comment191934>
Looks like you copied this from elsewhere, but it's odd that the other code used an std::move here, the rhs is already an rvalue.
src/tests/master_tests.cpp (lines 3723 - 3725)
<https://reviews.apache.org/r/45572/#comment191967>
Any reason this test was not placed directly above the TaskLabels test?
src/tests/master_tests.cpp (lines 3764 - 3766)
<https://reviews.apache.org/r/45572/#comment191950>
Thanks for the key / value naming!
src/tests/master_tests.cpp (lines 3774 - 3778)
<https://reviews.apache.org/r/45572/#comment191968>
Why is this needed?
src/tests/master_tests.cpp (line 3801)
<https://reviews.apache.org/r/45572/#comment191981>
We have the -> operator on Future/Option/Try/Result which tidies up the .get(). syntax used here:
```
Try<JSON::Object> parse = JSON::parse<JSON::Object>(response->body);
```
We haven't done a sweep yet to update existing code, but we want to use it going forward.
src/tests/master_tests.cpp (line 3806)
<https://reviews.apache.org/r/45572/#comment191977>
This should be an assert rather than an expect, since the test cannot continue if it fails.
src/tests/master_tests.cpp (line 3808)
<https://reviews.apache.org/r/45572/#comment191979>
Since we have the -> operator on result, I would suggest we just do `s/find/labels_/` and avoid the labelsObject temporary altogether. labelsObject was a poor name since it's a JSON array.
src/tests/master_tests.cpp (lines 3812 - 3814)
<https://reviews.apache.org/r/45572/#comment191980>
Perhaps this was copy paste, usually we wrap at the paren when wrapping on the next line looks more jagged, so:
```
EXPECT_EQ(JSON::Value(JSON::protobuf(createLabel("key1", "value1"))),
labels_->values[0]);
```
src/tests/oversubscription_tests.cpp (lines 230 - 231)
<https://reviews.apache.org/r/45572/#comment191949>
My suggestion for earlier was just to avoid using words like "foo" and "bar" in tests in favor of meaningful words like "key" and "value". Ditto below.
- Ben Mahler
On April 12, 2016, 4:59 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 4:59 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG 1f0527e86e333970f7f7879bb2bcbc33f0f44bc3
> include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861
> include/mesos/v1/mesos.proto a60a834e2538d54db7f257a0d4adfbb503ec1b0f
> src/common/http.cpp 735796cc9c5dfc8b27d9507e4a9a0659809b6f0d
> src/slave/http.cpp a684ff504535e2b9b2064f8048f5e03a0efcb059
> src/tests/master_tests.cpp a5b21d3d60f944fd52ceacb4bbbad2613f384db7
> src/tests/oversubscription_tests.cpp 23671746da2ac505d75bc2bd59114697d9161d52
> src/tests/slave_tests.cpp 4a576b98d1cc58072626ac2c41c599bd3c8385c5
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/
-----------------------------------------------------------
(Updated April 12, 2016, 4:59 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Review comments, and new ExecutorLabels test for both master and slave endpoint.
Bugs: MESOS-5029
https://issues.apache.org/jira/browse/MESOS-5029
Repository: mesos
Description
-------
Add labels to ExecutorInfo and deprecate source.
Diffs (updated)
-----
CHANGELOG 1f0527e86e333970f7f7879bb2bcbc33f0f44bc3
include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861
include/mesos/v1/mesos.proto a60a834e2538d54db7f257a0d4adfbb503ec1b0f
src/common/http.cpp 735796cc9c5dfc8b27d9507e4a9a0659809b6f0d
src/slave/http.cpp a684ff504535e2b9b2064f8048f5e03a0efcb059
src/tests/master_tests.cpp a5b21d3d60f944fd52ceacb4bbbad2613f384db7
src/tests/oversubscription_tests.cpp 23671746da2ac505d75bc2bd59114697d9161d52
src/tests/slave_tests.cpp 4a576b98d1cc58072626ac2c41c599bd3c8385c5
Diff: https://reviews.apache.org/r/45572/diff/
Testing
-------
Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
Thanks,
Zhitao Li
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Ben Mahler <be...@gmail.com>.
> On April 4, 2016, 5:26 p.m., haosdent huang wrote:
> > CHANGELOG, line 49
> > <https://reviews.apache.org/r/45572/diff/1/?file=1321727#file1321727line49>
> >
> > Usually we don't need change file. It would be changed by release manager.
>
> Zhitao Li wrote:
> hmm, right now the top section of CHANGELOG is written by each feature author in git blame, and this was also my impression that we should collectively help on changelog documentation so work load of release manager can be reduced.
>
> Please let me know if my understanding of the process is incorrect and I'll happily drop this change.
We do want contributors adding to the CHANGELOG deprecations / api changes / etc. It is difficult for release managers to do a good job at it given how many commits are in a release.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review126862
-----------------------------------------------------------
On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
> include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
> include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
> src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 4, 2016, 5:26 p.m., haosdent huang wrote:
> > CHANGELOG, line 49
> > <https://reviews.apache.org/r/45572/diff/1/?file=1321727#file1321727line49>
> >
> > Usually we don't need change file. It would be changed by release manager.
hmm, right now the top section of CHANGELOG is written by each feature author in git blame, and this was also my impression that we should collectively help on changelog documentation so work load of release manager can be reduced.
Please let me know if my understanding of the process is incorrect and I'll happily drop this change.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review126862
-----------------------------------------------------------
On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
> include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
> include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
> src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review126862
-----------------------------------------------------------
CHANGELOG (line 49)
<https://reviews.apache.org/r/45572/#comment189952>
Usually we don't need change file. It would be changed by release manager.
- haosdent huang
On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
> include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
> include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
> src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 9, 2016, 12:16 a.m., Ben Mahler wrote:
> > Looks pretty good, thanks!
> >
> > Could you also add tests that mirror the task label tests?
> >
> > ```
> > $ grep -R TEST src/tests | grep TaskLabels
> > src/tests/master_tests.cpp:TEST_F(MasterTest, TaskLabels)
> > src/tests/slave_tests.cpp:TEST_F(SlaveTest, TaskLabels)
> > ```
It seems like `/state` endpoint on master is constructed using the `Task` message in src/message/message.proto instead of `TaskInfo` in mesos.proto, so I guess I'd also need to define a `optional Labels executor_labels` field to the `Task` message in order to make executor_labels show up in master.
Does this sound right?
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review127918
-----------------------------------------------------------
On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
> include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
> include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
> src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review127918
-----------------------------------------------------------
Looks pretty good, thanks!
Could you also add tests that mirror the task label tests?
```
$ grep -R TEST src/tests | grep TaskLabels
src/tests/master_tests.cpp:TEST_F(MasterTest, TaskLabels)
src/tests/slave_tests.cpp:TEST_F(SlaveTest, TaskLabels)
```
CHANGELOG (lines 49 - 50)
<https://reviews.apache.org/r/45572/#comment191292>
The ExecutorInfo.source deprecation should be mentioned in the 'Deprecations' section. I would ok duplicating this in both sections, and only calling out the labels in the 'Additional API Changes' section.
include/mesos/mesos.proto (line 473)
<https://reviews.apache.org/r/45572/#comment191297>
No need to bother renaming slave to agent in the pre-V1 API.
include/mesos/mesos.proto (line 478)
<https://reviews.apache.org/r/45572/#comment191304>
All comments should end with a period, I guess you may have copied this from code that didn't follow that style.
include/mesos/mesos.proto (lines 496 - 498)
<https://reviews.apache.org/r/45572/#comment191311>
Any reason not to re-use the more descriptive comment on TaskInfo.labels? Specifically it would be nice to include the warning about the master keeping this data in memory.
- Ben Mahler
On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
> include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
> include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
> src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45572: Add labels to ExecutorInfo and deprecate
source.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45572/#review126538
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45572]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add labels to ExecutorInfo and deprecate source.
>
>
> Diffs
> -----
>
> CHANGELOG b90078d41357c29c9102df00a735bde460e797bb
> include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f
> include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84
> src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44
>
> Diff: https://reviews.apache.org/r/45572/diff/
>
>
> Testing
> -------
>
> Added a test in oversubciption_tests to make sure executor labels are visible to ResourceEstimator and QoSController.
>
>
> Thanks,
>
> Zhitao Li
>
>