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
---
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/#comment14
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
--- 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
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 18, 2015, 8:38 p.m.) Review request for mesos. Changes --- rebased Repository: mesos Description --- 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36580/#review92123 --- Patch looks great! Reviews applied: [36574, 36575, 36580] All tests passed. - Mesos ReviewBot On July 17, 2015, 7:56 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36580/ --- (Updated July 17, 2015, 7:56 p.m.) Review request for mesos. 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
