Re: Review Request 51866: Made the agent verify resource compatibility in checkpointResources().

[Jiang Yan Xu]

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

(Updated Sept. 16, 2016, 11:22 a.m.)


Review request for mesos, Anindya Sinha and Neil Conway.


Changes
---

Comments. NNFR.


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


Repository: mesos


Description
---

In handling CheckpointResourcesMessage, the agent should first make
sure the checkpointed resources are compatible before it syncs local
disk state.


Diffs (updated)
-

  src/slave/slave.cpp 7f99e4610d06ebadbef48ce314fec6ad04acb307 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 51866: Made the agent verify resource compatibility in checkpointResources().

[Anindya Sinha]

> On Sept. 14, 2016, 11:53 p.m., Anindya Sinha wrote:
> > src/slave/slave.cpp, line 5064
> > 
> >
> > Why is it verified against the old flag? `Slave::recover()` is the last 
> > step in `Slave::initialize()` by which time `info.resources()` reflects the 
> > `--resources` flag in this instance of mesos-slave process. So we are 
> > verifying with the new flag.
> 
> Jiang Yan Xu wrote:
> Since this is `recover()`, i.e., agent restart, I was referring to the 
> previous agent which laid down the checkpoint file before the restart but 
> would the following be more clear?
> 
> ```
> // This is to verify that the checkpointed resources are
> // compatible with the agent resources specified through the
> // '--resources' command line flag. The compatibility has been
> // verified by the old agent but the flag may have changed during
> // agent restart in an incompatible way and the operator may need
> // to either fix the flag or the checkpointed resources.
> ```

This looks way better. +1.


- Anindya


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


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51866/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In handling CheckpointResourcesMessage, the agent should first make
> sure the checkpointed resources are compatible before it syncs local
> disk state.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
> 
> Diff: https://reviews.apache.org/r/51866/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51866: Made the agent verify resource compatibility in checkpointResources().

[Jiang Yan Xu]

> On Sept. 14, 2016, 4:53 p.m., Anindya Sinha wrote:
> > src/slave/slave.cpp, line 5064
> > 
> >
> > Why is it verified against the old flag? `Slave::recover()` is the last 
> > step in `Slave::initialize()` by which time `info.resources()` reflects the 
> > `--resources` flag in this instance of mesos-slave process. So we are 
> > verifying with the new flag.

Since this is `recover()`, i.e., agent restart, I was referring to the previous 
agent which laid down the checkpoint file before the restart but would the 
following be more clear?

```
// This is to verify that the checkpointed resources are
// compatible with the agent resources specified through the
// '--resources' command line flag. The compatibility has been
// verified by the old agent but the flag may have changed during
// agent restart in an incompatible way and the operator may need
// to either fix the flag or the checkpointed resources.
```


- Jiang Yan


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


On Sept. 14, 2016, 11:21 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51866/
> ---
> 
> (Updated Sept. 14, 2016, 11:21 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In handling CheckpointResourcesMessage, the agent should first make
> sure the checkpointed resources are compatible before it syncs local
> disk state.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
> 
> Diff: https://reviews.apache.org/r/51866/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51866: Made the agent verify resource compatibility in checkpointResources().

[Anindya Sinha]

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




src/slave/slave.cpp (line 5064)


Why is it verified against the old flag? `Slave::recover()` is the last 
step in `Slave::initialize()` by which time `info.resources()` reflects the 
`--resources` flag in this instance of mesos-slave process. So we are verifying 
with the new flag.


- Anindya Sinha


On Sept. 14, 2016, 6:21 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51866/
> ---
> 
> (Updated Sept. 14, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In handling CheckpointResourcesMessage, the agent should first make
> sure the checkpointed resources are compatible before it syncs local
> disk state.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
> 
> Diff: https://reviews.apache.org/r/51866/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>