Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

> On April 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 426-429
> > 
> >
> > What about using reference here?
> > 
> > const ExecutorInfo& executorInfo = recoverable.executor_info();
> > const ContainerID& containerId = recoverable.container_id();
> > const FrameworkID& frameworkId = executorInfo.framework_id();
> > const ExecutorID& executorId = executorInfo.executor_id();
> 
> haosdent huang wrote:
> I remember mesos style guide didn't alllow const temporary refrence. 
> https://issues.apache.org/jira/browse/MESOS-2629
> 
> Guangya Liu wrote:
> But I saw that `We allow capturing non-temporaries by *constant 
> reference* when the intent is to **alias**.`, here most of the variables are 
> some **alias** I think. There are indeed some places using such style, such 
> as 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L80

I think it may breake this rule
```
// 4: Don't use (since the T that got constructed is a temporary!).
const T& t = T("Hello").member();
```
in style guide. But I would update this because it really din't bring any hurts 
here if use `const &`.


- haosdent


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


On April 4, 2016, 4:47 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44822/
> ---
> 
> (Updated April 4, 2016, 4:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till 
> Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove `SlaveState` in `MesosContainerizer` during recover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/44822/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

(Updated April 4, 2016, 4:47 p.m.)


Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, 
and Timothy Chen.


Changes
---

Address @gyliu's comments.


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


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

> On April 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, lines 180-182
> > 
> >
> > one line
> 
> haosdent huang wrote:
> The remain spaces is not allowed to one line here. Or I missunderstanding 
> anything here?
> 
> Guangya Liu wrote:
> Not quite get your point, what about following?
> 
>   process::Future _recover(const hashset& orphans);

`const list& recoverables` is necessary here. So could move 
change the interface to `process::Future _recover(const 
hashset& orphans);`


- haosdent


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


On April 3, 2016, 2:11 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44822/
> ---
> 
> (Updated April 3, 2016, 2:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till 
> Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove `SlaveState` in `MesosContainerizer` during recover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/44822/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[Guangya Liu]

> On 四月 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, lines 180-182
> > 
> >
> > one line
> 
> haosdent huang wrote:
> The remain spaces is not allowed to one line here. Or I missunderstanding 
> anything here?

Not quite get your point, what about following?

  process::Future _recover(const hashset& orphans);


> On 四月 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 426-429
> > 
> >
> > What about using reference here?
> > 
> > const ExecutorInfo& executorInfo = recoverable.executor_info();
> > const ContainerID& containerId = recoverable.container_id();
> > const FrameworkID& frameworkId = executorInfo.framework_id();
> > const ExecutorID& executorId = executorInfo.executor_id();
> 
> haosdent huang wrote:
> I remember mesos style guide didn't alllow const temporary refrence. 
> https://issues.apache.org/jira/browse/MESOS-2629

But I saw that `We allow capturing non-temporaries by *constant reference* when 
the intent is to **alias**.`, here most of the variables are some **alias** I 
think. There are indeed some places using such style, such as 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L80


- Guangya


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


On 四月 3, 2016, 2:11 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44822/
> ---
> 
> (Updated 四月 3, 2016, 2:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till 
> Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove `SlaveState` in `MesosContainerizer` during recover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/44822/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

> On April 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, lines 180-182
> > 
> >
> > one line

The remain spaces is not allowed to one line here. Or I missunderstanding 
anything here?


> On April 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 424
> > 
> >
> > What about s/newRecoverables/mesosRevocables or 
> > s/newRecoverables/filterRevocables and add some comments here for why do we 
> > need filter here? 
> > 
> > // ContainerState revocabale info which only
> > // include mesos containerizers.
> > list filterRecoverables;

`filterRevocables` more meaningful, thanks a lot!


> On April 3, 2016, 2:08 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 426-429
> > 
> >
> > What about using reference here?
> > 
> > const ExecutorInfo& executorInfo = recoverable.executor_info();
> > const ContainerID& containerId = recoverable.container_id();
> > const FrameworkID& frameworkId = executorInfo.framework_id();
> > const ExecutorID& executorId = executorInfo.executor_id();

I remember mesos style guide didn't alllow const temporary refrence. 
https://issues.apache.org/jira/browse/MESOS-2629


- haosdent


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


On April 3, 2016, 2:11 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44822/
> ---
> 
> (Updated April 3, 2016, 2:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till 
> Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-5085
> https://issues.apache.org/jira/browse/MESOS-5085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove `SlaveState` in `MesosContainerizer` during recover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/44822/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

(Updated April 3, 2016, 2:11 p.m.)


Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, Till Toenshoff, 
and Timothy Chen.


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


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[Guangya Liu]

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




src/slave/containerizer/mesos/containerizer.hpp (lines 180 - 182)


one line



src/slave/containerizer/mesos/containerizer.cpp (line 419)


What about s/newRecoverables/mesosRevocables or 
s/newRecoverables/filterRevocables and add some comments here for why do we 
need filter here? 

// ContainerState revocabale info which only
// include mesos containerizers.
list filterRecoverables;



src/slave/containerizer/mesos/containerizer.cpp (lines 421 - 424)


What about using reference here?

const ExecutorInfo& executorInfo = recoverable.executor_info();
const ContainerID& containerId = recoverable.container_id();
const FrameworkID& frameworkId = executorInfo.framework_id();
const ExecutorID& executorId = executorInfo.executor_id();


- Guangya Liu


On 四月 3, 2016, 1:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44822/
> ---
> 
> (Updated 四月 3, 2016, 1:06 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove `SlaveState` in `MesosContainerizer` during recover.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/44822/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

(Updated April 3, 2016, 1:06 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase.


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
a6e3a8b43fb7591ff6f23b019a062696c8bfb719 
  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

(Updated March 16, 2016, 5:56 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase.


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
e849932ab558ccd792080433169ae50dc208a12e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

(Updated March 16, 2016, 11:21 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase.


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 

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


Testing
---


Thanks,

haosdent huang

Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

Review request for mesos.


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 

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


Testing
---


Thanks,

haosdent huang