You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Felix Abecassis <fe...@gmail.com> on 2015/09/14 19:38:42 UTC
Re: Review Request 38279: [MESOS-3366] Allow resources/attributes
discovery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/
-----------------------------------------------------------
(Updated Sept. 14, 2015, 10:38 a.m.)
Review request for mesos, Connor Doyle and Niklas Nielsen.
Changes
-------
Adding myself as reviewer
Repository: mesos
Description
-------
First API draft for MESOS-3366.
1) Only supports resources for now, we can add another hook for attributes with a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
Diffs
-----
include/mesos/hook.hpp d90bacc
src/examples/test_hook_module.cpp bc13a8a
src/hook/manager.hpp 30d8321
src/hook/manager.cpp 754c238
src/slave/slave.cpp 5e5522e
Diff: https://reviews.apache.org/r/38279/diff/
Testing
-------
make clean && make && make check
Thanks,
Felix Abecassis
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Guangya Liu <gy...@gmail.com>.
> On 九月 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > <https://reviews.apache.org/r/38279/diff/2/?file=1077263#file1077263line659>
> >
> > s/VerifySlaveResourcesHook/VerifySlaveResourcesDiscoverHook/g
>
> Felix Abecassis wrote:
> I will rename it to "VerifySlaveInitializationResourcesDecorator" if that's OK, it will be more consistent with the other tests.
It seems a bit longer, not sure if we can change slaveInitializationResourcesDecorator to slaveResourcesDecorator? Why you are adding "Initialization" in this function?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99733
-----------------------------------------------------------
On 九月 22, 2015, 1:26 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated 九月 22, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Guangya Liu <gy...@gmail.com>.
> On 九月 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > <https://reviews.apache.org/r/38279/diff/2/?file=1077263#file1077263line659>
> >
> > s/VerifySlaveResourcesHook/VerifySlaveResourcesDiscoverHook/g
>
> Felix Abecassis wrote:
> I will rename it to "VerifySlaveInitializationResourcesDecorator" if that's OK, it will be more consistent with the other tests.
>
> Guangya Liu wrote:
> It seems a bit longer, not sure if we can change slaveInitializationResourcesDecorator to slaveResourcesDecorator? Why you are adding "Initialization" in this function?
>
> Felix Abecassis wrote:
> That's the name of the hook. Niklas asked me to refine the initial name.
I see. Thanks. Perhaps you also need to update the summary as this RR is only handling resource discovery but no attribute discovery.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99733
-----------------------------------------------------------
On 九月 22, 2015, 1:26 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated 九月 22, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fa...@nvidia.com>.
> On Sept. 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > <https://reviews.apache.org/r/38279/diff/2/?file=1077263#file1077263line659>
> >
> > s/VerifySlaveResourcesHook/VerifySlaveResourcesDiscoverHook/g
>
> Felix Abecassis wrote:
> I will rename it to "VerifySlaveInitializationResourcesDecorator" if that's OK, it will be more consistent with the other tests.
>
> Guangya Liu wrote:
> It seems a bit longer, not sure if we can change slaveInitializationResourcesDecorator to slaveResourcesDecorator? Why you are adding "Initialization" in this function?
That's the name of the hook. Niklas asked me to refine the initial name.
- Felix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99733
-----------------------------------------------------------
On Sept. 22, 2015, 1:26 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fe...@gmail.com>.
> On Sept. 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > <https://reviews.apache.org/r/38279/diff/2/?file=1077263#file1077263line659>
> >
> > s/VerifySlaveResourcesHook/VerifySlaveResourcesDiscoverHook/g
I will rename it to "VerifySlaveInitializationResourcesDecorator" if that's OK, it will be more consistent with the other tests.
- Felix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99733
-----------------------------------------------------------
On Sept. 19, 2015, 12:51 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2015, 12:51 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp ad710d7
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99733
-----------------------------------------------------------
src/tests/hook_tests.cpp (line 659)
<https://reviews.apache.org/r/38279/#comment156713>
s/VerifySlaveResourcesHook/VerifySlaveResourcesDiscoverHook/g
- Guangya Liu
On 九月 19, 2015, 12:51 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated 九月 19, 2015, 12:51 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp ad710d7
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Add a new callback enabling custom resource
discovery logic
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review102137
-----------------------------------------------------------
Ship it!
I'll go ahead and fix the last two issues before committing. Thanks for your patience Felix!
include/mesos/hook.hpp (lines 117 - 121)
<https://reviews.apache.org/r/38279/#comment159703>
Will wrap at 80 cols after recent style guide change.
include/mesos/hook.hpp (line 122)
<https://reviews.apache.org/r/38279/#comment159700>
Changing 'InitializationResourcesDecorator' -> 'ResourcesDecorator'
- Niklas Nielsen
On Sept. 21, 2015, 7:14 p.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 7:14 p.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Add a new callback enabling custom resource
discovery logic
Posted by Felix Abecassis <fa...@nvidia.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/
-----------------------------------------------------------
(Updated Sept. 22, 2015, 2:14 a.m.)
Review request for mesos, Connor Doyle and Niklas Nielsen.
Summary (updated)
-----------------
Add a new callback enabling custom resource discovery logic
Bugs: MESOS-3366
https://issues.apache.org/jira/browse/MESOS-3366
Repository: mesos
Description
-------
First API draft for MESOS-3366.
1) Only supports resources for now, we can add another hook for attributes with a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
Diffs
-----
include/mesos/hook.hpp 2353602
src/examples/test_hook_module.cpp 0dc74d6
src/hook/manager.hpp a517c05
src/hook/manager.cpp 691976e
src/slave/slave.cpp 29865ec
src/tests/hook_tests.cpp b23a587
Diff: https://reviews.apache.org/r/38279/diff/
Testing
-------
make clean && make && make check
Thanks,
Felix Abecassis
Re: Review Request 38279: Add a new callback enabling custom resource
discovery logic
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Sept. 21, 2015, 9:33 p.m., Kapil Arya wrote:
> > src/tests/hook_tests.cpp, line 695
> > <https://reviews.apache.org/r/38279/diff/2/?file=1077263#file1077263line695>
> >
> > Is it worth checking that resources indeed contains a "foo" before validating the value of "foo"?
>
> Felix Abecassis wrote:
> Not sure what you mean, do you want to add another assertion?
> If there is no "foo" here, won't the test fail cleanly anyway?
Sorry about missing this message earlier. Yeah, I just meant to include an additional assert for a better error message in case of failure.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99892
-----------------------------------------------------------
On Sept. 21, 2015, 10:14 p.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 10:14 p.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fa...@nvidia.com>.
> On Sept. 22, 2015, 1:33 a.m., Kapil Arya wrote:
> > src/tests/hook_tests.cpp, line 695
> > <https://reviews.apache.org/r/38279/diff/2/?file=1077263#file1077263line695>
> >
> > Is it worth checking that resources indeed contains a "foo" before validating the value of "foo"?
Not sure what you mean, do you want to add another assertion?
If there is no "foo" here, won't the test fail cleanly anyway?
- Felix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99892
-----------------------------------------------------------
On Sept. 22, 2015, 1:26 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99892
-----------------------------------------------------------
src/hook/manager.cpp (line 270)
<https://reviews.apache.org/r/38279/#comment156907>
It might be worth creating a ticket to handle this one so that the hooks are executed in order.
src/tests/hook_tests.cpp (line 695)
<https://reviews.apache.org/r/38279/#comment156908>
Is it worth checking that resources indeed contains a "foo" before validating the value of "foo"?
- Kapil Arya
On Sept. 21, 2015, 9:26 p.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:26 p.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp 29865ec
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fa...@nvidia.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/
-----------------------------------------------------------
(Updated Sept. 22, 2015, 1:26 a.m.)
Review request for mesos, Connor Doyle and Niklas Nielsen.
Bugs: MESOS-3366
https://issues.apache.org/jira/browse/MESOS-3366
Repository: mesos
Description
-------
First API draft for MESOS-3366.
1) Only supports resources for now, we can add another hook for attributes with a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
Diffs (updated)
-----
include/mesos/hook.hpp 2353602
src/examples/test_hook_module.cpp 0dc74d6
src/hook/manager.hpp a517c05
src/hook/manager.cpp 691976e
src/slave/slave.cpp 29865ec
src/tests/hook_tests.cpp b23a587
Diff: https://reviews.apache.org/r/38279/diff/
Testing
-------
make clean && make && make check
Thanks,
Felix Abecassis
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99647
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38279]
All tests passed.
- Mesos ReviewBot
On Sept. 19, 2015, 12:51 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2015, 12:51 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp ad710d7
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fe...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/
-----------------------------------------------------------
(Updated Sept. 19, 2015, 12:51 a.m.)
Review request for mesos, Connor Doyle and Niklas Nielsen.
Changes
-------
Added tests, changed the name of the hook.
Bugs: MESOS-3366
https://issues.apache.org/jira/browse/MESOS-3366
Repository: mesos
Description
-------
First API draft for MESOS-3366.
1) Only supports resources for now, we can add another hook for attributes with a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
Diffs (updated)
-----
include/mesos/hook.hpp 2353602
src/examples/test_hook_module.cpp 0dc74d6
src/hook/manager.hpp a517c05
src/hook/manager.cpp 691976e
src/slave/slave.cpp ad710d7
src/tests/hook_tests.cpp b23a587
Diff: https://reviews.apache.org/r/38279/diff/
Testing
-------
make clean && make && make check
Thanks,
Felix Abecassis
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99422
-----------------------------------------------------------
Bad patch!
Reviews applied: [38279]
Failed command: ./support/apply-review.sh -n -r 38279
Error:
2015-09-17 19:08:55 URL:https://reviews.apache.org/r/38279/diff/raw/ [4094/4094] -> "38279.patch" [1]
error: patch failed: src/examples/test_hook_module.cpp:193
error: src/examples/test_hook_module.cpp: patch does not apply
error: patch failed: src/hook/manager.hpp:72
error: src/hook/manager.hpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Sept. 14, 2015, 5:39 p.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 5:39 p.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp d90bacc
> src/examples/test_hook_module.cpp bc13a8a
> src/hook/manager.hpp 30d8321
> src/hook/manager.cpp 754c238
> src/slave/slave.cpp 5e5522e
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fe...@gmail.com>.
> On Sept. 14, 2015, 6:12 p.m., Connor Doyle wrote:
> > src/hook/manager.cpp, line 261
> > <https://reviews.apache.org/r/38279/diff/1/?file=1067842#file1067842line261>
> >
> > Please also add a test that verifies that the hooks are executed in the expected order. For example, have two hooks that both modify the `cpus` resource and verify that the second value is indeed retained.
Postponed for now.
- Felix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98886
-----------------------------------------------------------
On Sept. 19, 2015, 12:51 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2015, 12:51 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp 2353602
> src/examples/test_hook_module.cpp 0dc74d6
> src/hook/manager.hpp a517c05
> src/hook/manager.cpp 691976e
> src/slave/slave.cpp ad710d7
> src/tests/hook_tests.cpp b23a587
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Connor Doyle <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98886
-----------------------------------------------------------
src/hook/manager.cpp (line 261)
<https://reviews.apache.org/r/38279/#comment155558>
Please also add a test that verifies that the hooks are executed in the expected order. For example, have two hooks that both modify the `cpus` resource and verify that the second value is indeed retained.
- Connor Doyle
On Sept. 14, 2015, 5:39 p.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 5:39 p.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp d90bacc
> src/examples/test_hook_module.cpp bc13a8a
> src/hook/manager.hpp 30d8321
> src/hook/manager.cpp 754c238
> src/slave/slave.cpp 5e5522e
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Connor Doyle <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98884
-----------------------------------------------------------
src/examples/test_hook_module.cpp (line 208)
<https://reviews.apache.org/r/38279/#comment155545>
Please add a test to verify that the modified resources are reflected in the resource offers formed by the master. There are some good examples to follow in `src/tests/hook_tests.cpp`.
- Connor Doyle
On Sept. 14, 2015, 5:39 p.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 5:39 p.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp d90bacc
> src/examples/test_hook_module.cpp bc13a8a
> src/hook/manager.hpp 30d8321
> src/hook/manager.cpp 754c238
> src/slave/slave.cpp 5e5522e
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Sept. 14, 2015, 1:20 p.m., Niklas Nielsen wrote:
> > Per Connors comment, we need a test to exercise the new code
Ping - let's get this in soon :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98879
-----------------------------------------------------------
On Sept. 14, 2015, 10:39 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 10:39 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp d90bacc
> src/examples/test_hook_module.cpp bc13a8a
> src/hook/manager.hpp 30d8321
> src/hook/manager.cpp 754c238
> src/slave/slave.cpp 5e5522e
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98879
-----------------------------------------------------------
Per Connors comment, we need a test to exercise the new code
include/mesos/hook.hpp (line 116)
<https://reviews.apache.org/r/38279/#comment155532>
Think we need to sharpen the naming of the hook; the prepended 'slave' just indicates in which component i.e. not in the master. So it reads 'Resources Decorator' seems a bit too broad to me.
How about 'InitialResourcesDecorator' or something that indicates that it's during initialization.
src/slave/slave.cpp (line 383)
<https://reviews.apache.org/r/38279/#comment155628>
newline
- Niklas Nielsen
On Sept. 14, 2015, 10:39 a.m., Felix Abecassis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 10:39 a.m.)
>
>
> Review request for mesos, Connor Doyle and Niklas Nielsen.
>
>
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First API draft for MESOS-3366.
>
> 1) Only supports resources for now, we can add another hook for attributes with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
>
>
> Diffs
> -----
>
> include/mesos/hook.hpp d90bacc
> src/examples/test_hook_module.cpp bc13a8a
> src/hook/manager.hpp 30d8321
> src/hook/manager.cpp 754c238
> src/slave/slave.cpp 5e5522e
>
> Diff: https://reviews.apache.org/r/38279/diff/
>
>
> Testing
> -------
>
> make clean && make && make check
>
>
> Thanks,
>
> Felix Abecassis
>
>
Re: Review Request 38279: Enabled resources/attributes discovery
Posted by Felix Abecassis <fe...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/
-----------------------------------------------------------
(Updated Sept. 14, 2015, 10:39 a.m.)
Review request for mesos, Connor Doyle and Niklas Nielsen.
Summary (updated)
-----------------
Enabled resources/attributes discovery
Bugs: MESOS-3366
https://issues.apache.org/jira/browse/MESOS-3366
Repository: mesos
Description
-------
First API draft for MESOS-3366.
1) Only supports resources for now, we can add another hook for attributes with a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by previous callbacks and are free to override or merge the detected resources as they see fit.
Diffs
-----
include/mesos/hook.hpp d90bacc
src/examples/test_hook_module.cpp bc13a8a
src/hook/manager.hpp 30d8321
src/hook/manager.cpp 754c238
src/slave/slave.cpp 5e5522e
Diff: https://reviews.apache.org/r/38279/diff/
Testing
-------
make clean && make && make check
Thanks,
Felix Abecassis