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