You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/02/01 02:03:15 UTC
Review Request 65447: Refactored couple of launch task sanity checks
into a single code path.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65447/
-----------------------------------------------------------
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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65447/#comment276571>
Most of our member function names start with verbs. Maybe `checkXXXXX`? Also we cannot tell what kind of invariants are checked from the name.
src/slave/slave.cpp
Lines 2071-2079 (original), 2056-2064 (patched)
<https://reviews.apache.org/r/65447/#comment276569>
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)
<https://reviews.apache.org/r/65447/#comment276597>
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.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65447/#comment276406>
This comment doesn't make sense here?
src/slave/slave.cpp
Lines 2071-2079 (original), 2056-2064 (patched)
<https://reviews.apache.org/r/65447/#comment276407>
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)
<https://reviews.apache.org/r/65447/#comment276408>
each arg on a different line.
src/slave/slave.cpp
Lines 2123 (patched)
<https://reviews.apache.org/r/65447/#comment276409>
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
>
>