You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Megha Sharma <ms...@apple.com> on 2017/06/15 17:12:36 UTC
Review Request 60105: Added helper method recoverSlaveState.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Here, a helper method recoverSlaveState is added to
determine if the slave state is to be recovered or not.
During agent recovery, if the SlaveInfo compatibility
check fails and the agent is rebooted then to be
backwards compatible with MESOS <= 1.3, since a
a rebooted agent didn't recover state, we clear
slave state and slaveId so the agent registers with the
master as a new agent.
Diffs
-----
src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d
src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6
Diff: https://reviews.apache.org/r/60105/diff/1/
Testing
-------
Thanks,
Megha Sharma
Re: Review Request 60105: Added helper method recoverSlaveState.
Posted by Megha Sharma <ms...@apple.com>.
> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 5945-5946 (patched)
> > <https://reviews.apache.org/r/60105/diff/3/?file=1752079#file1752079line5945>
> >
> > This comment seems to be misplaced, why does the code here cares? It just processes the `slaveState` which could be optional regardless of what we have done for the reboot case.
Some of the comments I added were for my own undrestanding (like this one) which I forgot to clean up before pushing
> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Line 5950 (original), 5969-5974 (patched)
> > <https://reviews.apache.org/r/60105/diff/3/?file=1752079#file1752079line5969>
> >
> > As suggested by related comments, we are not removing the symlink here so we shouldn't need to comment about it here.
> >
> > "This is being done ...": here the comment describes the low-level mechanics but perhaps we need a high level comment about "this" instead, which is that:
> >
> > Prior to Mesos 1.4 we bypass the state recovery and start as a new agent upon reboot (introduced in MESOS-844); starting in Mesos 1.4 we'll attempt to recover the slave state even after reboot (so we don't discard the agent ID unnecessarily) but in case of SlaveInfo mismatch we'll still continue as a new agent so we don't introduce a regression for the reboot scenario.
> >
> > We can go on to explain the rationale: agent resource and attribute changes are more often associated with host reboots and we don't want to agent to flap in such cases.
> >
> > But it's fine if we focus on improving the comments after we first get the code rigth.
Makes sense, updated the comment.
> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 5987-5989 (patched)
> > <https://reviews.apache.org/r/60105/diff/3/?file=1752079#file1752079line5987>
> >
> > You don't need to do this here. If you continue as a new agent, it's going to update the latest symlink when it's registered:
> >
> > https://github.com/apache/mesos/blob/3bea3fcb4eef00ce469ba4f27fcfd2d3eec05aea/src/slave/paths.cpp#L621-L622
Good Point, removed
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178388
-----------------------------------------------------------
On June 15, 2017, 7:31 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 15, 2017, 7:31 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Here, a helper method recoverSlaveState is added to
> determine if the slave state is to be recovered or not.
> During agent recovery, if the SlaveInfo compatibility
> check fails and the agent is rebooted then to be
> backwards compatible with MESOS <= 1.3, since a
> a rebooted agent didn't recover state, we clear
> slave state and slaveId so the agent registers with the
> master as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d
> src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6
>
>
> Diff: https://reviews.apache.org/r/60105/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 5798-5867 (original), 5798-5867 (patched)
> > <https://reviews.apache.org/r/60105/diff/3/?file=1752079#file1752079line5798>
> >
> > So all of this work is only useful for recovering frameworks, it looks like we don't need to do them before we decide we actually are going to recover the frameworks (i.e., not continue as a new agent)?
> >
> > This reasoning certainly also applies to the current HEAD but since we are donig refactor here, we should probably address this (in a separate preceding review perhaps)
>
> Megha Sharma wrote:
> I strongly agree we should defer doing this work until we start to recover the frameworks. I was thinking of doing this may be as another review for slave recovery improvements after this change is in.
Agreed to do in a separate refactoring/improvements patch may be after this one lands.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178388
-----------------------------------------------------------
On June 23, 2017, 5:19 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 5:19 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/7/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > PTAL at these comments but I realize for this review it's probably easier if we sync on the high-level strategy first.
Rearranged the code as per our discussion.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178388
-----------------------------------------------------------
On June 22, 2017, 11:38 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 11:38 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/6/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 21, 2017, 4:39 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 5798-5867 (original), 5798-5867 (patched)
> > <https://reviews.apache.org/r/60105/diff/3/?file=1752079#file1752079line5798>
> >
> > So all of this work is only useful for recovering frameworks, it looks like we don't need to do them before we decide we actually are going to recover the frameworks (i.e., not continue as a new agent)?
> >
> > This reasoning certainly also applies to the current HEAD but since we are donig refactor here, we should probably address this (in a separate preceding review perhaps)
I strongly agree we should defer doing this work until we start to recover the frameworks. I was thinking of doing this may be as another review for slave recovery improvements after this change is in.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178388
-----------------------------------------------------------
On June 21, 2017, 11:22 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 21, 2017, 11:22 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/5/
>
>
> Testing
> -------
>
> Latest make check results pending.
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Added helper method recoverSlaveState.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178388
-----------------------------------------------------------
PTAL at these comments but I realize for this review it's probably easier if we sync on the high-level strategy first.
src/slave/slave.cpp
Lines 5798-5867 (original), 5798-5867 (patched)
<https://reviews.apache.org/r/60105/#comment252425>
So all of this work is only useful for recovering frameworks, it looks like we don't need to do them before we decide we actually are going to recover the frameworks (i.e., not continue as a new agent)?
This reasoning certainly also applies to the current HEAD but since we are donig refactor here, we should probably address this (in a separate preceding review perhaps)
src/slave/slave.cpp
Lines 5945 (patched)
<https://reviews.apache.org/r/60105/#comment252322>
An empty blank line between multi-line blocks of code like this. Here and elsewhere.
src/slave/slave.cpp
Lines 5945-5946 (patched)
<https://reviews.apache.org/r/60105/#comment252512>
This comment seems to be misplaced, why does the code here cares? It just processes the `slaveState` which could be optional regardless of what we have done for the reboot case.
src/slave/slave.cpp
Lines 5959 (patched)
<https://reviews.apache.org/r/60105/#comment252326>
Open `{` on the next line.
src/slave/slave.cpp
Line 5941 (original), 5960 (patched)
<https://reviews.apache.org/r/60105/#comment252327>
The indentation of the whole methods seems off?
src/slave/slave.cpp
Line 5950 (original), 5969-5974 (patched)
<https://reviews.apache.org/r/60105/#comment252429>
As suggested by related comments, we are not removing the symlink here so we shouldn't need to comment about it here.
"This is being done ...": here the comment describes the low-level mechanics but perhaps we need a high level comment about "this" instead, which is that:
Prior to Mesos 1.4 we bypass the state recovery and start as a new agent upon reboot (introduced in MESOS-844); starting in Mesos 1.4 we'll attempt to recover the slave state even after reboot (so we don't discard the agent ID unnecessarily) but in case of SlaveInfo mismatch we'll still continue as a new agent so we don't introduce a regression for the reboot scenario.
We can go on to explain the rationale: agent resource and attribute changes are more often associated with host reboots and we don't want to agent to flap in such cases.
But it's fine if we focus on improving the comments after we first get the code rigth.
src/slave/slave.cpp
Lines 5975 (patched)
<https://reviews.apache.org/r/60105/#comment252422>
So at this point you still have the `slaveState` object and it has a `rebooted` field, why do we need another `rebooted` field in `recoveryInfo` if this is the only place it's used?
src/slave/slave.cpp
Lines 5976-5983 (patched)
<https://reviews.apache.org/r/60105/#comment252427>
We shouldn't need to construct the same message twice. If the same message can be used either for LOG(INFO) or Failure then we can construct the message in a single place first and decide how to use it / whether to add to it.
src/slave/slave.cpp
Lines 5987-5989 (patched)
<https://reviews.apache.org/r/60105/#comment252347>
You don't need to do this here. If you continue as a new agent, it's going to update the latest symlink when it's registered:
https://github.com/apache/mesos/blob/3bea3fcb4eef00ce469ba4f27fcfd2d3eec05aea/src/slave/paths.cpp#L621-L622
src/slave/slave.cpp
Lines 5992 (patched)
<https://reviews.apache.org/r/60105/#comment252435>
Would a None() not work?
Also, I know I originally suggested a helper method but given how it looks right now, perhaps it's less of a hassle if we keep it inline?
Even if we keep it as a helper, as a one-use helper whose semantics is so tightly coupled with the internals of `Slave::recover()`, perhaps it could just be done as a lambda.
```
Try<Nothing> adjustSlaveState = [&slaveState]() { ... }();
```
Let's try to inline it first?
src/slave/slave.cpp
Lines 5993-5995 (patched)
<https://reviews.apache.org/r/60105/#comment252526>
You have to clear it here because it's assigned above prematurely. As the comment above suggests, it's a hack for comparing the slave infos.
Pehaps we should just make a copy of the SlaveInfo above for comparision. If we are going to continue as a new agent, we are just **not** going to execute `info = slaveState->info.get();` below?
src/slave/slave.cpp
Line 5965 (original)
<https://reviews.apache.org/r/60105/#comment252444>
Why remove it?
src/slave/slave.cpp
Lines 6122-6125 (original)
<https://reviews.apache.org/r/60105/#comment252527>
Why remove it?
- Jiang Yan Xu
On June 15, 2017, 12:31 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 15, 2017, 12:31 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Here, a helper method recoverSlaveState is added to
> determine if the slave state is to be recovered or not.
> During agent recovery, if the SlaveInfo compatibility
> check fails and the agent is rebooted then to be
> backwards compatible with MESOS <= 1.3, since a
> a rebooted agent didn't recover state, we clear
> slave state and slaveId so the agent registers with the
> master as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d
> src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6
>
>
> Diff: https://reviews.apache.org/r/60105/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 23, 2017, 6:21 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Line 6003 (original), 6007 (patched)
> > <https://reviews.apache.org/r/60105/diff/6/?file=1759987#file1759987line6010>
> >
> > Empty line above.
This is not possible I guess, git pre-commit hook complains: Redundant blank line at the start of a code block should be deleted.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178750
-----------------------------------------------------------
On June 23, 2017, 9:30 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 9:30 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/8/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 23, 2017, 6:21 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 5994-5999 (patched)
> > <https://reviews.apache.org/r/60105/diff/6/?file=1759987#file1759987line5997>
> >
> > I tweaked it a little bit:
> >
> > ```
> > // Fail the recovery unless the agent is recovering for the first
> > // time after host reboot.
> > //
> > // Prior to Mesos 1.4 we directly bypass the state recovery and
> > // start as a new agent upon reboot (introduced in MESOS-844).
> > // This unncessarily discards the existing agent ID (MESOS-6223).
> > // Starting in Mesos 1.4 we'll attempt to recover the slave state
> > // even after reboot but in case of slave info mismatch we'll fall
> > // back to recovering as a new agent (existing behavior). This
> > // prevents the agent from flapping if the slave info (resources,
> > // attributes, etc.) change is due to host maintainance associated
> > // with the reboot.
> > ```
> >
> > What do you think? Feel free to improve on it.
+1, good and concise explanation about the changed behavior.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178750
-----------------------------------------------------------
On June 23, 2017, 5:19 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 5:19 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/7/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 23, 2017, 11:21 a.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Line 6003 (original), 6007 (patched)
> > <https://reviews.apache.org/r/60105/diff/6/?file=1759987#file1759987line6010>
> >
> > Empty line above.
>
> Megha Sharma wrote:
> This is not possible I guess, git pre-commit hook complains: Redundant blank line at the start of a code block should be deleted.
OK let's see the change. it's not the start of a code block in the verison I commented on. :)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178750
-----------------------------------------------------------
On June 23, 2017, 2:30 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 2:30 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/8/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178750
-----------------------------------------------------------
src/slave/slave.cpp
Lines 5980-5981 (original), 5980-5981 (patched)
<https://reviews.apache.org/r/60105/#comment252968>
```
SlaveInfo _info(info);
```
We typically add a prefix for two variations of the same thing.
src/slave/slave.cpp
Line 5985 (original), 5985 (patched)
<https://reviews.apache.org/r/60105/#comment252969>
Nit: s/errorMessage/message/
There should be no confusion here with a shorter name.
src/slave/slave.cpp
Lines 5994-5999 (patched)
<https://reviews.apache.org/r/60105/#comment253029>
I tweaked it a little bit:
```
// Fail the recovery unless the agent is recovering for the first
// time after host reboot.
//
// Prior to Mesos 1.4 we directly bypass the state recovery and
// start as a new agent upon reboot (introduced in MESOS-844).
// This unncessarily discards the existing agent ID (MESOS-6223).
// Starting in Mesos 1.4 we'll attempt to recover the slave state
// even after reboot but in case of slave info mismatch we'll fall
// back to recovering as a new agent (existing behavior). This
// prevents the agent from flapping if the slave info (resources,
// attributes, etc.) change is due to host maintainance associated
// with the reboot.
```
What do you think? Feel free to improve on it.
src/slave/slave.cpp
Lines 6000 (patched)
<https://reviews.apache.org/r/60105/#comment253015>
`if (state.rebooted)` instead? (Whitespace and variable usage)
The above examples
```
Option<ResourcesState> resourcesState = state->resources;
Option<SlaveState> slaveState = state->slave;
```
help reduce line length and are used many times. It's not the same situation with `rebooted`.
src/slave/slave.cpp
Lines 5997-6003 (original), 6000-6016 (patched)
<https://reviews.apache.org/r/60105/#comment253031>
To avoid going to deep in the branching hierarchy perhaps the following is better?
```
if (!state.rebooted) {
return Failure(message);
}
LOG(WARNING) << "Falling back to recover as a new agent due to error: "
<< message;
// Cleaning up the slave state to avoid any state recovery for the
// old agent.
slaveState = None();
```
src/slave/slave.cpp
Lines 5997-6002 (original), 6001-6006 (patched)
<https://reviews.apache.org/r/60105/#comment253026>
This log message and metric increment used to occur outside
```
if (flags.recover == "reconnect" &&
!(infoCopy == slaveState->info.get())) {
```
but it's now changed.
How about we put it directly under
```
if (slaveState.isSome() && slaveState->info.isSome()) {
```
You can find similar handling above with `resourcesState`:
```
if (resourcesState.isSome()) {
if (resourcesState->errors > 0) {
LOG(WARNING) << "Errors encountered during resources recovery: "
<< resourcesState->errors;
metrics.recovery_errors += resourcesState->errors;
}
```
src/slave/slave.cpp
Line 6003 (original), 6007 (patched)
<https://reviews.apache.org/r/60105/#comment253017>
Empty line above.
src/slave/slave.cpp
Line 6004 (original), 6019 (patched)
<https://reviews.apache.org/r/60105/#comment253021>
Empty line above comments.
- Jiang Yan Xu
On June 23, 2017, 10:19 a.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 10:19 a.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/7/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review179817
-----------------------------------------------------------
Ship it!
We'll need to document this behavior change in CHANGELOG and upgrades.md.
- Jiang Yan Xu
On June 29, 2017, 4:42 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 29, 2017, 4:42 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/10/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review180073
-----------------------------------------------------------
Please comment with the result of a real reboot test.
- Jiang Yan Xu
On June 29, 2017, 4:42 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 29, 2017, 4:42 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/10/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 29, 2017, 11:42 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/10/
Changes: https://reviews.apache.org/r/60105/diff/9-10/
Testing
-------
make check done together with 60104 and 56895
Thanks,
Megha Sharma
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 29, 2017, 11:35 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/9/
Changes: https://reviews.apache.org/r/60105/diff/8-9/
Testing
-------
make check done together with 60104 and 56895
Thanks,
Megha Sharma
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 24, 2017, 6:19 a.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 6011-6020 (patched)
> > <https://reviews.apache.org/r/60105/diff/8/?file=1761637#file1761637line6019>
> >
> > Could you use the code in my comment:
> >
> > ```
> > if (!state->rebooted) {
> > return Failure(message);
> > }
> >
> > LOG(WARNING) << "Falling back to recover as a new agent due to error: "
> > << message;
> >
> > // Clean up the slave state to avoid any state recovery for the
> > // old agent.
> > slaveState = None();
> > ```
> >
> > I'll comment on the specific lines for the reasoning.
Aah my bad, will get rid of the else.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178830
-----------------------------------------------------------
On June 29, 2017, 11:35 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 29, 2017, 11:35 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/9/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
> On June 24, 2017, 6:19 a.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 6014 (patched)
> > <https://reviews.apache.org/r/60105/diff/8/?file=1761637#file1761637line6022>
> >
> > s/INFO/WARNING/.
Changed to warning. I was debating earlier between warning and info and felt info was more apt since mesos is already doing the remediation here.
- Megha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178830
-----------------------------------------------------------
On June 29, 2017, 11:35 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 29, 2017, 11:35 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/9/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/#review178830
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/slave.cpp
Lines 6011-6020 (patched)
<https://reviews.apache.org/r/60105/#comment253074>
Could you use the code in my comment:
```
if (!state->rebooted) {
return Failure(message);
}
LOG(WARNING) << "Falling back to recover as a new agent due to error: "
<< message;
// Clean up the slave state to avoid any state recovery for the
// old agent.
slaveState = None();
```
I'll comment on the specific lines for the reasoning.
src/slave/slave.cpp
Lines 6013 (patched)
<https://reviews.apache.org/r/60105/#comment253075>
`else`: I was suggesting pulling up the direct return case so we can avoid another `else` block. Normally one style is not necessary superior than the other but in this case it helps avoid three levels if `if ... else` branching.
src/slave/slave.cpp
Lines 6014 (patched)
<https://reviews.apache.org/r/60105/#comment253076>
s/INFO/WARNING/.
src/slave/slave.cpp
Lines 6015 (patched)
<https://reviews.apache.org/r/60105/#comment253078>
No `\n` necessary.
src/slave/slave.cpp
Lines 6016 (patched)
<https://reviews.apache.org/r/60105/#comment253079>
We normally in logs first state what action is taken and then attach the error after `: `. In this case since the error has multiple lines, let's state the action first so it's not buried by the error.
- Jiang Yan Xu
On June 23, 2017, 2:30 p.m., Megha Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60105/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 2:30 p.m.)
>
>
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to Mesos 1.4 we bypass the state recovery and
> start as a new agent upon reboot. Starting in Mesos 1.4
> we'll attempt to recover the slave state even after reboot
> except for when there is a SlaveInfo mismatch.
> Here, we cleanup the slave state for a rebooted agent if
> there's been a SlaveInfo mismatch during recovery to
> ensure that no other state is recovered and the
> agent enventually registers as a new agent.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
>
>
> Diff: https://reviews.apache.org/r/60105/diff/8/
>
>
> Testing
> -------
>
> make check done together with 60104 and 56895
>
>
> Thanks,
>
> Megha Sharma
>
>
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 23, 2017, 9:30 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/8/
Changes: https://reviews.apache.org/r/60105/diff/7-8/
Testing
-------
make check done together with 60104 and 56895
Thanks,
Megha Sharma
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 23, 2017, 5:19 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/7/
Changes: https://reviews.apache.org/r/60105/diff/6-7/
Testing
-------
make check done together with 60104 and 56895
Thanks,
Megha Sharma
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 22, 2017, 11:38 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/6/
Changes: https://reviews.apache.org/r/60105/diff/5-6/
Testing
-------
make check done together with 60104 and 56895
Thanks,
Megha Sharma
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 22, 2017, 9:16 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/5/
Testing (updated)
-------
make check done together with 60104 and 56895
Thanks,
Megha Sharma
Re: Review Request 60105: Clean rebooted slave's state if slaveInfo
mismatches.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 21, 2017, 11:22 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Summary (updated)
-----------------
Clean rebooted slave's state if slaveInfo mismatches.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/5/
Changes: https://reviews.apache.org/r/60105/diff/4-5/
Testing (updated)
-------
Latest make check results pending.
Thanks,
Megha Sharma
Re: Review Request 60105: Added helper method recoverSlaveState.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 21, 2017, 11:11 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description (updated)
-------
Prior to Mesos 1.4 we bypass the state recovery and
start as a new agent upon reboot. Starting in Mesos 1.4
we'll attempt to recover the slave state even after reboot
except for when there is a SlaveInfo mismatch.
Here, we cleanup the slave state for a rebooted agent if
there's been a SlaveInfo mismatch during recovery to
ensure that no other state is recovered and the
agent enventually registers as a new agent.
Diffs (updated)
-----
src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c
Diff: https://reviews.apache.org/r/60105/diff/4/
Changes: https://reviews.apache.org/r/60105/diff/3-4/
Testing
-------
Thanks,
Megha Sharma
Re: Review Request 60105: Added helper method recoverSlaveState.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 15, 2017, 7:31 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Here, a helper method recoverSlaveState is added to
determine if the slave state is to be recovered or not.
During agent recovery, if the SlaveInfo compatibility
check fails and the agent is rebooted then to be
backwards compatible with MESOS <= 1.3, since a
a rebooted agent didn't recover state, we clear
slave state and slaveId so the agent registers with the
master as a new agent.
Diffs (updated)
-----
src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d
src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6
Diff: https://reviews.apache.org/r/60105/diff/3/
Changes: https://reviews.apache.org/r/60105/diff/2-3/
Testing
-------
Thanks,
Megha Sharma
Re: Review Request 60105: Added helper method recoverSlaveState.
Posted by Megha Sharma <ms...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60105/
-----------------------------------------------------------
(Updated June 15, 2017, 5:53 p.m.)
Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223
Repository: mesos
Description
-------
Here, a helper method recoverSlaveState is added to
determine if the slave state is to be recovered or not.
During agent recovery, if the SlaveInfo compatibility
check fails and the agent is rebooted then to be
backwards compatible with MESOS <= 1.3, since a
a rebooted agent didn't recover state, we clear
slave state and slaveId so the agent registers with the
master as a new agent.
Diffs (updated)
-----
src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d
src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6
Diff: https://reviews.apache.org/r/60105/diff/2/
Changes: https://reviews.apache.org/r/60105/diff/1-2/
Testing
-------
Thanks,
Megha Sharma