Re: Review Request 38279: Enabled resources/attributes discovery

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99733
---



src/tests/hook_tests.cpp (line 659)


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: Enabled resources/attributes discovery

[Guangya Liu]

> On 九月 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > 
> >
> > 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

[Felix Abecassis]

> On Sept. 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > 
> >
> > 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

[Kapil Arya]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review99892
---



src/hook/manager.cpp (line 270)


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)


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

[Guangya Liu]

> On 九月 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > 
> >
> > 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

[Felix Abecassis]

> On Sept. 21, 2015, 7:57 a.m., Guangya Liu wrote:
> > src/tests/hook_tests.cpp, line 659
> > 
> >
> > 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

[Felix Abecassis]

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

[Felix Abecassis]

> On Sept. 22, 2015, 1:33 a.m., Kapil Arya wrote:
> > src/tests/hook_tests.cpp, line 695
> > 
> >
> > 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

[Connor Doyle]

> On Sept. 14, 2015, 5:34 p.m., Connor Doyle wrote:
> > src/hook/manager.cpp, line 261
> > 
> >
> > Please add a comment describing how the order of hook execution is 
> > determined.  e.g. order in which they appear in the modules json passed to 
> > the slave, etc.
> 
> Felix Abecassis wrote:
> Actually, it seems there is no predefined order right now since 
> src/hook/manager.cpp is using a hashmap, should I open another issue to 
> discuss how to fix this?

Yes, please do.  A HashMap might be overkill for that use case.  Maybe the hook 
manager can get by with a simpler order-preserving data structure.  As modules 
become more common, operators will probably want to control the invocation 
order.  For now the flag value is the only control surface exposed.


- Connor


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98860
---


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

[Felix Abecassis]

> On Sept. 14, 2015, 5:34 p.m., Connor Doyle wrote:
> > src/hook/manager.cpp, line 261
> > 
> >
> > Please add a comment describing how the order of hook execution is 
> > determined.  e.g. order in which they appear in the modules json passed to 
> > the slave, etc.

Actually, it seems there is no predefined order right now since 
src/hook/manager.cpp is using a hashmap, should I open another issue to discuss 
how to fix this?


- Felix


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98860
---


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

[Felix Abecassis]

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

[Felix Abecassis]

> On Sept. 14, 2015, 6:12 p.m., Connor Doyle wrote:
> > src/hook/manager.cpp, line 261
> > 
> >
> > 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

[Mesos ReviewBot]

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

[Mesos ReviewBot]

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

[Niklas Nielsen]

> 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

[Felix Abecassis]

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

Re: Review Request 38279: Enabled resources/attributes discovery

[Niklas Nielsen]

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


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)


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

[Connor Doyle]

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


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

[Connor Doyle]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38279/#review98886
---



src/hook/manager.cpp (line 261)


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