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