----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45932/#review127933 -----------------------------------------------------------
Thanks! Looking pretty good, some minor comments below. CHANGELOG (line 52) <https://reviews.apache.org/r/45932/#comment191319> How about: ``` Add task information to container resource usage information. ``` include/mesos/mesos.proto (lines 1026 - 1027) <https://reviews.apache.org/r/45932/#comment191312> We should avoid saying "used" here in favor of "allocated". We can probably do without this comment. How about s/resources/allocated here to match the executor above? include/mesos/mesos.proto (line 1029) <https://reviews.apache.org/r/45932/#comment191313> This comment doesn't seem to add anything? include/mesos/mesos.proto (line 1032) <https://reviews.apache.org/r/45932/#comment191315> Can we omit this? Why did you include it? include/mesos/mesos.proto (lines 1035 - 1036) <https://reviews.apache.org/r/45932/#comment191314> Avoid saying "running" here since there are more non-terminal states than just RUNNING: ``` // Non-terminal tasks. ``` src/slave/slave.cpp (line 5169) <https://reviews.apache.org/r/45932/#comment191316> let's say "non-terminal" instead of running, ideally slave.hpp doesn't say "running" either, but let's leave it for now src/slave/slave.cpp (lines 5171 - 5176) <https://reviews.apache.org/r/45932/#comment191317> How about s/taskEntry/t/ ? src/tests/oversubscription_tests.cpp (lines 230 - 231) <https://reviews.apache.org/r/45932/#comment191318> How about s/task_label/key/ s/task_label_value/value/ ? Will it fit on one line then? We generally avoid "foo" and "bar" in favor of things like "key" and "value" to make the test clearer, so please ignore the executor labels here. - Ben Mahler On April 8, 2016, 5:19 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45932/ > ----------------------------------------------------------- > > (Updated April 8, 2016, 5:19 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-5030 > https://issues.apache.org/jira/browse/MESOS-5030 > > > Repository: mesos > > > Description > ------- > > Add stripped TaskInfo's to ResourceUsage.Executor message. > > > Diffs > ----- > > CHANGELOG 4553465cc3dc17956f168469d405f7a453d6359e > include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f > include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 > src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 > src/tests/oversubscription_tests.cpp > 23671746da2ac505d75bc2bd59114697d9161d52 > > Diff: https://reviews.apache.org/r/45932/diff/ > > > Testing > ------- > > Added new test to verify ResourceUsage sees task labels. > > > Thanks, > > Zhitao Li > >
