Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-09-06 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On Sept. 6, 2017, 11:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated Sept. 6, 2017, 11:59 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp da9dff1e9bb18780ac5a5b25d2fd98e931da74af 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/resource_provider_manager_tests.cpp 
> 83a1340fa16b19e3297a8c0ca413afc312de00ec 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-09-06 Thread Jan Schlicht 


> On Aug. 8, 2017, 3:53 p.m., Jan Schlicht wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Lines 383 (patched)
> > 
> >
> > Nit: Use `v1::ResourceProviderInfo` instead of 
> > `mesos::v1::ResourceProviderInfo` to be consistent with the use of the `v1` 
> > namespace in this test.
> 
> Benjamin Bannier wrote:
> This does not work for my compiler and requires explicit disambiguation 
> (e.g., `no type named 'ResourceProviderInfo' in namespace 
> 'mesos::internal::tests::v1'; did you mean simply 'ResourceProviderInfo'?`). 
> Note that for other `v1` types in this file we typically import the name 
> directly into the current scope with a using decl.

You're right, I even ran into that in my patches.


- Jan


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


On Sept. 6, 2017, 1:59 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated Sept. 6, 2017, 1:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp da9dff1e9bb18780ac5a5b25d2fd98e931da74af 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/resource_provider_manager_tests.cpp 
> 83a1340fa16b19e3297a8c0ca413afc312de00ec 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-09-06 Thread Benjamin Bannier 


> On Aug. 8, 2017, 3:53 p.m., Jan Schlicht wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Lines 383 (patched)
> > 
> >
> > Nit: Use `v1::ResourceProviderInfo` instead of 
> > `mesos::v1::ResourceProviderInfo` to be consistent with the use of the `v1` 
> > namespace in this test.

This does not work for my compiler and requires explicit disambiguation (e.g., 
`no type named 'ResourceProviderInfo' in namespace 
'mesos::internal::tests::v1'; did you mean simply 'ResourceProviderInfo'?`). 
Note that for other `v1` types in this file we typically import the name 
directly into the current scope with a using decl.


> On Aug. 8, 2017, 3:53 p.m., Jan Schlicht wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Lines 394 (patched)
> > 
> >
> > See nit above.

See above.


- Benjamin


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


On Sept. 6, 2017, 1:59 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated Sept. 6, 2017, 1:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp da9dff1e9bb18780ac5a5b25d2fd98e931da74af 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/resource_provider_manager_tests.cpp 
> 83a1340fa16b19e3297a8c0ca413afc312de00ec 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-09-06 Thread Benjamin Bannier 


> On Aug. 1, 2017, 12:48 a.m., Jie Yu wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Lines 134 (patched)
> > 
> >
> > In fact, what's the different between this and 
> > ResourceProviderHttpApiTest.Subscribe.
> > 
> > My suggestion is:
> > 
> > Let's move all resource provider manager related tests into a single 
> > test file (resource_provider_manager_tests.cpp). Use different TestCase 
> > name to group them.
> > 
> > For HTTP level tests (e.g., content type, accept type, etc.), call it 
> > ResourceProviderManagerHttpApiTest.
> > 
> > For all the functional tests, use the manager interface directly (call 
> > it ResourceProviderManagerTest).

Agreed. I incorporated the changes into the existing test.


- Benjamin


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


On Sept. 6, 2017, 1:59 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated Sept. 6, 2017, 1:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp da9dff1e9bb18780ac5a5b25d2fd98e931da74af 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/resource_provider_manager_tests.cpp 
> 83a1340fa16b19e3297a8c0ca413afc312de00ec 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-09-06 Thread Benjamin Bannier 

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

(Updated Sept. 6, 2017, 1:59 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Extended existing test; rebased.


Bugs: MESOS-7837
https://issues.apache.org/jira/browse/MESOS-7837


Repository: mesos


Description
---

In order to inform users of resource provider managers that the
managed resources have changed, we add a new 'ResourceProviderMessage'
type for communicating changes to the managed total resources.

We add code to trigger sending of that message when a resource
provider subscribes with the manager.

In the future this message could also be used to communicate changes
to an already subscribed resource provider's total resources.


Diffs (updated)
-

  src/resource_provider/manager.cpp da9dff1e9bb18780ac5a5b25d2fd98e931da74af 
  src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
  src/tests/resource_provider_manager_tests.cpp 
83a1340fa16b19e3297a8c0ca413afc312de00ec 


Diff: https://reviews.apache.org/r/61182/diff/4/

Changes: https://reviews.apache.org/r/61182/diff/3-4/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-08-17 Thread Jie Yu 

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



Can you rebase?

- Jie Yu


On Aug. 1, 2017, 5:42 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated Aug. 1, 2017, 5:42 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/resource_provider_manager_tests.cpp 
> 85906ea5e1bb3516ef264de22913ce0a3c9c58c5 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-08-08 Thread Jan Schlicht 

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



Looks great, only nitpicking here. Please rebase, `resource_provider::Driver` 
has been changed to allow reconnections using an `EndpointDetector`.


src/tests/resource_provider_manager_tests.cpp
Lines 383 (patched)


Nit: Use `v1::ResourceProviderInfo` instead of 
`mesos::v1::ResourceProviderInfo` to be consistent with the use of the `v1` 
namespace in this test.



src/tests/resource_provider_manager_tests.cpp
Lines 394 (patched)


See nit above.


- Jan Schlicht


On Aug. 1, 2017, 7:42 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated Aug. 1, 2017, 7:42 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/resource_provider_manager_tests.cpp 
> 85906ea5e1bb3516ef264de22913ce0a3c9c58c5 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-08-01 Thread Benjamin Bannier 

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

(Updated Aug. 1, 2017, 7:42 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Used a single file for RP manager-related tests; used `MockResourceProvider`; 
rebased.


Bugs: MESOS-7837
https://issues.apache.org/jira/browse/MESOS-7837


Repository: mesos


Description
---

In order to inform users of resource provider managers that the
managed resources have changed, we add a new 'ResourceProviderMessage'
type for communicating changes to the managed total resources.

We add code to trigger sending of that message when a resource
provider subscribes with the manager.

In the future this message could also be used to communicate changes
to an already subscribed resource provider's total resources.


Diffs (updated)
-

  src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
  src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
  src/tests/resource_provider_manager_tests.cpp 
85906ea5e1bb3516ef264de22913ce0a3c9c58c5 


Diff: https://reviews.apache.org/r/61182/diff/3/

Changes: https://reviews.apache.org/r/61182/diff/2-3/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-07-31 Thread Jie Yu 


> On July 28, 2017, 10:03 p.m., Jie Yu wrote:
> > src/resource_provider/message.hpp
> > Lines 29 (patched)
> > 
> >
> > With ERP in mind, maybe it's more approapriate to make this message for 
> > a single resource provider? For instance:
> > 
> > ```
> > struct ResourceProviderMessage
> > {
> >   enum class Type
> >   {
> > UPDATE_TOTAL_RESOURCES,
> >   };
> >   
> >   struct UpdateTotalResources
> >   {
> > Option id;
> > Resources total;
> >   };
> > 
> >   Type type;
> >   
> >   UpdateTotalResources updateTotalResources;
> > };
> > ```
> 
> Benjamin Bannier wrote:
> I am not sure we need to do this right now. Independent of ERP/LRP, 
> splitting this up along RPs would somewhat help us to
> 
> * keep the transfered `total` small, and
> * make it easier to perform RP-specific actions when a RP changes (e.g., 
> to implement simple resource health actions).
> 
> OTOH it should be possible to implement this without complicating the 
> protocol.
> 
> Could we punt on that one for now?
> 
> * * *
> 
> I took your suggestion and renamed `s/TOTAL/UPDATE_TOTAL_RESOURCES` in 
> this patch.

The reason I was pushing for per provider update message is because the agent 
logic will be different. If we agree that per provider update is the way to go 
in the future, we should just do it right now so that we don't need to adjust 
the agent logic too much in the future. what do you think?


- Jie


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


On July 31, 2017, 3:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated July 31, 2017, 3:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/CMakeLists.txt 6dd2716de942adf6cefa5a464ef664f3c3ebb7a3 
>   src/tests/resource_provider_http_api_tests.cpp 
> 85906ea5e1bb3516ef264de22913ce0a3c9c58c5 
>   src/tests/resource_provider_manager_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

2017-07-28 Thread Jie Yu 

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




src/resource_provider/message.hpp
Lines 29 (patched)


With ERP in mind, maybe it's more approapriate to make this message for a 
single resource provider? For instance:

```
struct ResourceProviderMessage
{
  enum class Type
  {
UPDATE_TOTAL_RESOURCES,
  };
  
  struct UpdateTotalResources
  {
Option id;
Resources total;
  };

  Type type;
  
  UpdateTotalResources updateTotalResources;
};
```


- Jie Yu


On July 27, 2017, 2:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated July 27, 2017, 2:49 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/CMakeLists.txt 6dd2716de942adf6cefa5a464ef664f3c3ebb7a3 
>   src/tests/resource_provider_http_api_tests.cpp 
> 85906ea5e1bb3516ef264de22913ce0a3c9c58c5 
>   src/tests/resource_provider_manager_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>