mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Megha Sharma <mshar...@apple.com>
Subject Re: Review Request 60105: Added helper method recoverSlaveState.
Date Wed, 21 Jun 2017 23:05:03 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message