Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

2019-05-21 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- Benjamin Mahler


On May 21, 2019, 1:40 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70665/
> ---
> 
> (Updated May 21, 2019, 1:40 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch consolidates the scattered logic of sending Framework updates
> (namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
> UpdateFrameworkMessage to agents).
> 
> NOTE: The UpdateFrameworkMessage is now also being sent in the case of
> non-`forced` subscription request from a framework which has already
> been registered. Previously it was not being sent in this case -
> this likely is a bug.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
>   src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
> 
> 
> Diff: https://reviews.apache.org/r/70665/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

2019-05-21 Thread Andrei Sekretenko 


> On May 20, 2019, 6:19 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 667 (patched)
> > 
> >
> > Why did this need to be passed separately? Is there some reason 
> > `framework->pid` can't be used?

I checked, looks like `framework->pid` is updated correctly on all the 
re-subscription paths. So there is no need to pass this separately. Switched to 
`framework->pid`.


> On May 20, 2019, 6:19 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2832-2835 (original), 2839-2842 (patched)
> > 
> >
> > This looks like a bug, the pid needs to be set here in the non http api 
> > case.

Certainly a bug.

Looks like we are not testing this anywhere.
There seems to be exactly one test which has checking UpdateFrameworkMessage 
among its purposes:
https://github.com/apache/mesos/blob/eecb82c77117998af0c67a53c64e9b1e975acfa4/src/tests/fault_tolerance_tests.cpp#L1432
I think I'll have to add a check for PID there.


- Andrei


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


On May 21, 2019, 1:40 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70665/
> ---
> 
> (Updated May 21, 2019, 1:40 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch consolidates the scattered logic of sending Framework updates
> (namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
> UpdateFrameworkMessage to agents).
> 
> NOTE: The UpdateFrameworkMessage is now also being sent in the case of
> non-`forced` subscription request from a framework which has already
> been registered. Previously it was not being sent in this case -
> this likely is a bug.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
>   src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
> 
> 
> Diff: https://reviews.apache.org/r/70665/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

2019-05-21 Thread Andrei Sekretenko 

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

(Updated May 21, 2019, 1:40 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch consolidates the scattered logic of sending Framework updates
(namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
UpdateFrameworkMessage to agents).

NOTE: The UpdateFrameworkMessage is now also being sent in the case of
non-`forced` subscription request from a framework which has already
been registered. Previously it was not being sent in this case -
this likely is a bug.


Diffs (updated)
-

  src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
  src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 


Diff: https://reviews.apache.org/r/70665/diff/2/

Changes: https://reviews.apache.org/r/70665/diff/1-2/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

2019-05-20 Thread Benjamin Mahler 

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




src/master/master.hpp
Lines 667 (patched)


Why did this need to be passed separately? Is there some reason 
`framework->pid` can't be used?



src/master/master.cpp
Lines 2819 (patched)


newline



src/master/master.cpp
Lines 2832-2835 (original), 2839-2842 (patched)


This looks like a bug, the pid needs to be set here in the non http api 
case.



src/master/master.cpp
Lines 3151 (patched)


newline


- Benjamin Mahler


On May 17, 2019, 3:03 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70665/
> ---
> 
> (Updated May 17, 2019, 3:03 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch consolidates the scattered logic of sending Framework updates
> (namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
> UpdateFrameworkMessage to agents).
> 
> NOTE: The UpdateFrameworkMessage is now also being sent in the case of
> non-`forced` subscription request from a framework which has already
> been registered. Previously it was not being sent in this case -
> this likely is a bug.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42 
>   src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 
> 
> 
> Diff: https://reviews.apache.org/r/70665/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

2019-05-17 Thread Andrei Sekretenko 

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

Review request for mesos.


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


Repository: mesos


Description
---

This patch consolidates the scattered logic of sending Framework updates
(namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
UpdateFrameworkMessage to agents).

NOTE: The UpdateFrameworkMessage is now also being sent in the case of
non-`forced` subscription request from a framework which has already
been registered. Previously it was not being sent in this case -
this likely is a bug.


Diffs
-

  src/master/master.hpp c523c937e294eaffe0e58306c267770c119c9f42 
  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 


Diff: https://reviews.apache.org/r/70665/diff/1/


Testing
---


Thanks,

Andrei Sekretenko