Re: Review Request 65447: Refactored couple of launch task sanity checks into a single code path.

[Chun-Hung Hsiao]

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




src/slave/slave.cpp
Line 2033 (original), 2033 (patched)


Most of our member function names start with verbs. Maybe `checkX`? 
Also we cannot tell what kind of invariants are checked from the name.



src/slave/slave.cpp
Lines 2071-2079 (original), 2056-2064 (patched)


Agreed. This confuses me a bit because before going through the code, I 
didn't know why we need to do the same check in `_run` and its continuation.



src/slave/slave.cpp
Lines 2196-2197 (original), 2213-2214 (patched)



http://mesos.apache.org/documentation/latest/c++-style-guide/#function-definition-invocation


- Chun-Hung Hsiao


On Feb. 1, 2018, 2:03 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65447/
> ---
> 
> (Updated Feb. 1, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial steps common to `_run()` and `__run()` on the task launch
> code path are refactored.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65447/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65447: Refactored couple of launch task sanity checks into a single code path.

[Vinod Kone]

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




src/slave/slave.cpp
Lines 2064-2065 (original), 2049-2050 (patched)


This comment doesn't make sense here?



src/slave/slave.cpp
Lines 2071-2079 (original), 2056-2064 (patched)


hmm. we are mutating state here in a function that looks like it is only 
doing checks. that's weird.

also, i don't know if i like this refactor because there are still some pre 
run checks being done in `_run` and `__run` themselves. it's not clear which 
ones this helper does and which ones it doesn't.

typically, we refactor common code into a function if that function makes 
sense by itself. i don't think that's the case here so i would actually propose 
that we do not refactor unless we find a good abstraction.



src/slave/slave.cpp
Lines 2117-2118 (patched)


each arg on a different line.



src/slave/slave.cpp
Lines 2123 (patched)


CHECK_NOTNULL(getFramework(frameworkId))


- Vinod Kone


On Feb. 1, 2018, 2:03 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65447/
> ---
> 
> (Updated Feb. 1, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial steps common to `_run()` and `__run()` on the task launch
> code path are refactored.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65447/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>