Re: Review Request 49607: Added support for recovered frameworks.

2016-07-05 Thread Vinod Kone 

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


Ship it!




Ship It!

- Vinod Kone


On July 5, 2016, 8:15 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49607/
> ---
> 
> (Updated July 5, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously after a master failover while agents
> were reregistering we might not have access
> to all FrameworkInfo (which would first be available
> once the Framework reregistered). This patch
> adds support that once a framework reregisters
> we also keep track of recovered FrameworkInfos.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/master/master.cpp 78a8889313179b4509a6657e6c61d9f6d3bb99c0 
>   src/messages/messages.proto 6d7eccc5501b3df010d981e09d0654e815da551f 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
> 
> Diff: https://reviews.apache.org/r/49607/diff/
> 
> 
> Testing
> ---
> 
> (sudo) make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49607: Added support for recovered frameworks.

2016-07-05 Thread Joerg Schad 


> On July 5, 2016, 6:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 5013-5042
> > 
> >
> > I think we should change this as follows for clarity:
> > 
> > ```
> > foreach (const FrameworkInfo& frameworkInfo, frameworks) {
> >   
> >   Framework* framework = getFramework(frameworkInfo.id());
> >   
> >   if (framework != nullptr) {
> > // Send UpdateFrameworkMessage
> >   } else {
> > // Add it recovered map.
> >   }
> > 
> > }
> > 
> > ```

We cannot collapse the loops yet for backwards compatability (old agent, new 
master). I added a TODO to remove it after the deprecation cycle.


- Joerg


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


On July 5, 2016, 8:15 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49607/
> ---
> 
> (Updated July 5, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously after a master failover while agents
> were reregistering we might not have access
> to all FrameworkInfo (which would first be available
> once the Framework reregistered). This patch
> adds support that once a framework reregisters
> we also keep track of recovered FrameworkInfos.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/master/master.cpp 78a8889313179b4509a6657e6c61d9f6d3bb99c0 
>   src/messages/messages.proto 6d7eccc5501b3df010d981e09d0654e815da551f 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
> 
> Diff: https://reviews.apache.org/r/49607/diff/
> 
> 
> Testing
> ---
> 
> (sudo) make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49607: Added support for recovered frameworks.

2016-07-05 Thread Joerg Schad 

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

(Updated July 5, 2016, 8:15 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Previously after a master failover while agents
were reregistering we might not have access
to all FrameworkInfo (which would first be available
once the Framework reregistered). This patch
adds support that once a framework reregisters
we also keep track of recovered FrameworkInfos.


Diffs (updated)
-

  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/master/master.cpp 78a8889313179b4509a6657e6c61d9f6d3bb99c0 
  src/messages/messages.proto 6d7eccc5501b3df010d981e09d0654e815da551f 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 

Diff: https://reviews.apache.org/r/49607/diff/


Testing
---

(sudo) make check


Thanks,

Joerg Schad

Re: Review Request 49607: Added support for recovered frameworks.

2016-07-05 Thread Joerg Schad 

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

(Updated July 5, 2016, 7:51 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Previously after a master failover while agents
were reregistering we might not have access
to all FrameworkInfo (which would first be available
once the Framework reregistered). This patch
adds support that once a framework reregisters
we also keep track of recovered FrameworkInfos.


Diffs (updated)
-

  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/master/master.cpp 78a8889313179b4509a6657e6c61d9f6d3bb99c0 
  src/messages/messages.proto 6d7eccc5501b3df010d981e09d0654e815da551f 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 

Diff: https://reviews.apache.org/r/49607/diff/


Testing
---

(sudo) make check


Thanks,

Joerg Schad

Re: Review Request 49607: Added support for recovered frameworks.

2016-07-05 Thread Vinod Kone 

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




src/master/master.hpp (lines 483 - 484)


swap the order of the arguments.

s/active//

here and everywhere else.

is this backwards comptabile? old master <-> new agent and new master <-> 
old agent?



src/master/master.hpp (line 1680)


Can you add a comment here on what `recovered` represents?



src/master/master.cpp (lines 922 - 923)


swap the order.

s/active_//



src/master/master.cpp (lines 4757 - 4758)


swap the order.



src/master/master.cpp (lines 4942 - 4943)


swap and s/active//



src/master/master.cpp (lines 5013 - 5042)


I think we should change this as follows for clarity:

```
foreach (const FrameworkInfo& frameworkInfo, frameworks) {
  
  Framework* framework = getFramework(frameworkInfo.id());
  
  if (framework != nullptr) {
// Send UpdateFrameworkMessage
  } else {
// Add it recovered map.
  }

}

```



src/master/master.cpp (line 5034)


// If a framework known to the agent has not yet re-registered with the 
master
// add it to the `recovered` map.



src/messages/messages.proto (lines 421 - 422)


swap the order.

also s/active_frameworks/frameworks/



src/slave/slave.cpp (lines 1411 - 1413)


move this TODO to #1416


- Vinod Kone


On July 5, 2016, 8:43 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49607/
> ---
> 
> (Updated July 5, 2016, 8:43 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously after a master failover while agents
> were reregistering we might not have access
> to all FrameworkInfo (which would first be available
> once the Framework reregistered). This patch
> adds support that once a framework reregisters
> we also keep track of recovered FrameworkInfos.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/master/master.cpp 78a8889313179b4509a6657e6c61d9f6d3bb99c0 
>   src/messages/messages.proto 6d7eccc5501b3df010d981e09d0654e815da551f 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
> 
> Diff: https://reviews.apache.org/r/49607/diff/
> 
> 
> Testing
> ---
> 
> (sudo) make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>