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 2015/07/21 18:46:35 UTC
Re: Review Request 36580: Added TaskStatus label decorator hook for
Slave
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36580/
-----------------------------------------------------------
(Updated July 21, 2015, 12:46 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen.
Changes
-------
removed master hook; addressed BenH's comments.
Summary (updated)
-----------------
Added TaskStatus label decorator hook for Slave
Repository: mesos
Description (updated)
-------
This allows Slave modules to expose some information to the
frameworks as well as Mesos-DNS via state.json.
Diffs (updated)
-----
include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f
src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab
src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc
src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf
src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db
Diff: https://reviews.apache.org/r/36580/diff/
Testing
-------
make check with a new hook test.
Thanks,
Kapil Arya
Re: Review Request 36580: Added TaskStatus label decorator hook for
Slave
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36580/#review92467
-----------------------------------------------------------
Ship it!
src/examples/test_hook_module.cpp (line 166)
<https://reviews.apache.org/r/36580/#comment146668>
I'd explicitly call out the test, so that a reader of the code can understand the global invariant here.
src/hook/manager.cpp (line 212)
<https://reviews.apache.org/r/36580/#comment146665>
Why do we copy the result into a TaskStatus only to return it below? seems like the code here should just be:
if (result.isSome()) {
return result.get();
} else if (result.isError()) {
LOG(WARNING) << "Slave TaskStatus label decorator hook failed for "
<< "module '" << name << "': " << result.error();
}
return status.labels();
And then we can take in TaskStatus as a const&. I see that this pattern is used everywhere so let's not do this now but unless I'm missing something we should circle back.
src/tests/hook_tests.cpp (line 509)
<https://reviews.apache.org/r/36580/#comment146666>
Here and everywhere else in this review and other reviews, the expected value comes first, and the actual value comes second. Again, since all this code looks like this we can take care of it in a subsequent clean up review.
- Benjamin Hindman
On July 21, 2015, 4:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36580/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 4:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows Slave modules to expose some information to the
> frameworks as well as Mesos-DNS via state.json.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f
> src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab
> src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc
> src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
> src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db
>
> Diff: https://reviews.apache.org/r/36580/diff/
>
>
> Testing
> -------
>
> make check with a new hook test.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 36580: Added TaskStatus label decorator hook for
Slave
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36580/#review92513
-----------------------------------------------------------
src/hook/manager.cpp (line 212)
<https://reviews.apache.org/r/36580/#comment146723>
There are a couple of reasons for the pattern (here and in TaskInfo):
1. A decorator hooks can also act as "undecorator" hooks, i.e., it is allowed to remove labels from the given TaskStatus/TaskInfo. Thus everytime a hook is called, it is called with the current state of TaskInfo/TaskStatus Labels.
2. There can be multiple hooks for label decoration, so we can't simply return on `result.isSome()`. We need to accumulate all of those.
- Kapil Arya
On July 21, 2015, 12:46 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36580/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 12:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows Slave modules to expose some information to the
> frameworks as well as Mesos-DNS via state.json.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f
> src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab
> src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc
> src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
> src/tests/hook_tests.cpp 09205fb89925c22b1157294a756db87d911a63db
>
> Diff: https://reviews.apache.org/r/36580/diff/
>
>
> Testing
> -------
>
> make check with a new hook test.
>
>
> Thanks,
>
> Kapil Arya
>
>