You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by bin zheng <zh...@chinaunicom.cn> on 2018/06/13 06:17:02 UTC
Review Request 67567: Rename variable names in slave.hpp to be more
explicit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/
-----------------------------------------------------------
Review request for mesos and Gilbert Song.
Repository: mesos
Description
-------
Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
Diffs
-----
src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
Diff: https://reviews.apache.org/r/67567/diff/1/
Testing
-------
Thanks,
bin zheng
Re: Review Request 67567: Rename variable names in slave.hpp to be
more explicit.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/#review204665
-----------------------------------------------------------
PASS: Mesos patch 67567 was successfully built and tested.
Reviews applied: `['67538', '67567']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67567
- Mesos Reviewbot Windows
On June 13, 2018, 6:17 a.m., bin zheng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67567/
> -----------------------------------------------------------
>
> (Updated June 13, 2018, 6:17 a.m.)
>
>
> Review request for mesos and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
>
>
> Diffs
> -----
>
> src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
> src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
> src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
>
>
> Diff: https://reviews.apache.org/r/67567/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bin zheng
>
>
Re: Review Request 67567: Rename variable names in slave.hpp to be
more explicit.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/#review204712
-----------------------------------------------------------
I wonder if this is really more explicit? Right now it seems very likely to me that e.g., `Slave::info` would refer to a `SlaveInfo`; calling it `Slave::slaveInfo` doesn't seem to add much. There also seems to be no need to disambiguate here as the datstructures touched here do not contain more that a single `*Info` value.
Note that we use a similar pattern in e.g., `master.hpp` and `hierarchical.hpp`.
- Benjamin Bannier
On June 13, 2018, 7:14 p.m., bin zheng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67567/
> -----------------------------------------------------------
>
> (Updated June 13, 2018, 7:14 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Bugs: MESOS-8680
> https://issues.apache.org/jira/browse/MESOS-8680
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
>
>
> Diffs
> -----
>
> src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
> src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
> src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
>
>
> Diff: https://reviews.apache.org/r/67567/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bin zheng
>
>
Re: Review Request 67567: Rename variable names in slave.hpp to be
more explicit.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/#review204729
-----------------------------------------------------------
Just did a partial review. Thanks for the work for these tedious renaming!
But I do same a similar concern with Benjamin that not all of these renamings will improve readability, and they introduce inconsistencies in the codebase.
src/slave/http.cpp
Lines 221 (patched)
<https://reviews.apache.org/r/67567/#comment287417>
When breaking an expression into two lines, we only indent by 2 spaces:
```
exectuor_->executorInfo.resoures()
.begin()->allocation_info().role());
```
src/slave/http.cpp
Lines 236 (patched)
<https://reviews.apache.org/r/67567/#comment287416>
The way we break function calls into multiple lines in `if` statements is not well laid out, but generally we do the following indentation style:
```
if (!approvers_->approved<VIEW_TASK>(
*task, framework_->frameworkInfo)){
```
In other words, we indent the parameters by 4 spaces after aligning with the parenthesis of the `if` statement. This would make the following code more readable:
```
if (!approvers_->approved<VIEW_TASK>(
*task, framework_->frameworkInfo) &&
<another_condition>...) {
```
src/slave/http.cpp
Lines 247 (patched)
<https://reviews.apache.org/r/67567/#comment287418>
Similarly, let's indent by two more spaces.
Same below.
src/slave/http.cpp
Lines 1435 (patched)
<https://reviews.apache.org/r/67567/#comment287421>
1 more space here.
src/slave/http.cpp
Lines 1694 (patched)
<https://reviews.apache.org/r/67567/#comment287422>
One mero space here. Same below.
src/slave/slave.hpp
Line 724 (original), 724 (patched)
<https://reviews.apache.org/r/67567/#comment287415>
To many spaces. We usually leave only one space between the type and the variable name:
```
Salevinfo slaveInfo;
```
- Chun-Hung Hsiao
On June 13, 2018, 5:14 p.m., bin zheng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67567/
> -----------------------------------------------------------
>
> (Updated June 13, 2018, 5:14 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Bugs: MESOS-8680
> https://issues.apache.org/jira/browse/MESOS-8680
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
>
>
> Diffs
> -----
>
> src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
> src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
> src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
>
>
> Diff: https://reviews.apache.org/r/67567/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bin zheng
>
>
Re: Review Request 67567: Renamed variable names in slave.hpp to be
more explicit.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/#review204740
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['67567']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67567
Relevant logs:
- [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67567/logs/mesos-tests-stdout.log):
```
[ RUN ] SlaveTest.ResourceVersions
[ OK ] SlaveTest.ResourceVersions (200 ms)
[ RUN ] SlaveTest.ReconfigurationPolicy
[ OK ] SlaveTest.ReconfigurationPolicy (292 ms)
[ RUN ] SlaveTest.ResourceProviderReconciliation
[ OK ] SlaveTest.ResourceProviderReconciliation (348 ms)
[ RUN ] SlaveTest.RunTaskResourceVersions
[ OK ] SlaveTest.RunTaskResourceVersions (315 ms)
[----------] 83 tests from SlaveTest (64461 ms total)
[----------] 3 tests from SlaveStateTest
[ RUN ] SlaveStateTest.CheckpointString
[ OK ] SlaveStateTest.CheckpointString (4 ms)
[ RUN ] SlaveStateTest.CheckpointProtobufMessage
[ OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
[ RUN ] SlaveStateTest.CheckpointRepeatedProtobufMessages
[ OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (11 ms)
[----------] 3 tests from SlaveStateTest (27 ms total)
[----------] 30 tests from SlaveRecoveryTest/0, where TypeParam = class mesos::internal::slave::MesosContainerizer
[ RUN ] SlaveRecoveryTest/0.RecoverSlaveState
[ OK ] SlaveRecoveryTest/0.RecoverSlaveState (820 ms)
[ RUN ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
[ OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (2985 ms)
[ RUN ] SlaveRecoveryTest/0.ReconnectExecutor
[ OK ] SlaveRecoveryTest/0.ReconnectExecutor (2885 ms)
[ RUN ] SlaveRecoveryTest/0.ReconnectExecutorRetry
[ OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (852 ms)
[ RUN ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
```
- [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67567/logs/mesos-tests-stderr.log):
```
@ 00007FF78416A8E7 ??
@ 00007FF784029A58 std::_Invoker_functor::_Call<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,process::ProcessBase *>
@ 00007FF7840BD4E8 std::invoke<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,process::ProcessBase *>
@ 00007FF7840D22CB lambda::internal::Partial<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,std::_Ph<1> >::invoke_expand<<lambda_9f5bb6c728b761604e288ae85a7b250c>,std::tuple<process::Future<Option<mesos::MasterInfo> >,std::_Ph<1> >,st
@ 00007FF7840004FA )<process::ProcessBase *
@ 00007FF78403026C std::_Invoker_functor::_Call<lambda::internal::Partial<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,std::_Ph<1> >,process::ProcessBase *>
@ 00007FF7840C3B9C std::invoke<lambda::internal::Partial<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,std::_Ph<1> >,process::ProcessBase *>
@ 00007FF784009BF1 )<lambda::internal::Partial<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,std::_Ph<1> >,process::ProcessBase *
@ 00007FF7841760E6 process::ProcessBase *)>::CallableFn<lambda::internal::Partial<<lambda_9f5bb6c728b761604e288ae85a7b250c>,process::Future<Option<mesos::MasterInfo> >,std::_Ph<1> > >::operator(
@ 00007FF785B5E46D process::ProcessBase *)>::operator(
@ 00007FF785A0F509 process::ProcessBase::consume
@ 00007FF785BB7ADA process::DispatchEvent::consume
@ 00007FF781C27A67 process::ProcessBase::serve
@ 00007FF785A1D5C0 process::ProcessManager::resume
@ 00007FF785B4B581 ??
@ 00007FF785A6F340 std::_Invoker_functor::_Call<<lambda_124422ac022fa041208b80c1460630d7> >
@ 00007FF785ACFAF0 std::invoke<<lambda_124422ac022fa041208b80c1460630d7> >
@ 00007FF785A8497C std::_LaunchPad<std::unique_ptr<std::tuple<<lambda_124422ac022fa041208b80c1460630d7> >,std::default_delete<std::tuple<<lambda_124422ac022fa041208b80c1460630d7> > > > >::_Execute<0>
@ 00007FF785B9E81A std::_LaunchPad<std::unique_ptr<std::tuple<<lambda_124422ac022fa041208b80c1460630d7> >,std::default_delete<std::tuple<<lambda_124422ac022fa041208b80c1460630d7> > > > >::_Run
@ 00007FF785B8A088 std::_LaunchPad<std::unique_ptr<std::tuple<<lambda_124422ac022fa041208b80c1460630d7> >,std::default_delete<std::tuple<<lambda_124422ac022fa041208b80c1460630d7> > > > >::_Go
@ 00007FF785B705DD std::_Pad::_Call_func
@ 00007FFDA1733428 _register_onexit_function
@ 00007FFDA1733071 _register_onexit_function
@ 00007FFDB6231FE4 BaseThreadInitThunk
@ 00007FFDB6F4F061 RtlUserThreadStart
'c9e1e127-e2ad-4ced-8a08-54c29e5baa04' of framework e119e235-ce0c-48aa-928b-0f4f2df9bc31-0000 at executor(1)@192.10.1.5:59961
I0613 23:08:45.964924 3856 slave.cpp:4985] Received re-registration message from executor 'c9e1e127-e2ad-4ced-8a08-54c29e5baa04' of framework e119e235-ce0c-48aa-928b-0f4f2df9bc31-0000
I0613 23:08:45.968926 7636 slave.cpp:5903] No pings from master received within 75secs
F0613 23:08:45.970927 7636 slave.cpp:1249] Check failed: state == DISCONNECTED || state == RUNNING || state == TERMINATING RECOVERING
```
- Mesos Reviewbot Windows
On June 13, 2018, 9:47 p.m., bin zheng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67567/
> -----------------------------------------------------------
>
> (Updated June 13, 2018, 9:47 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
>
>
> Bugs: MESOS-8680
> https://issues.apache.org/jira/browse/MESOS-8680
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
>
>
> Diffs
> -----
>
> src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
> src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
> src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
>
>
> Diff: https://reviews.apache.org/r/67567/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bin zheng
>
>
Re: Review Request 67567: Renamed variable names in slave.hpp to be
more explicit.
Posted by bin zheng <zh...@chinaunicom.cn>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/
-----------------------------------------------------------
(Updated 六月 13, 2018, 9:47 p.m.)
Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
Changes
-------
Fixed code format error
Summary (updated)
-----------------
Renamed variable names in slave.hpp to be more explicit.
Bugs: MESOS-8680
https://issues.apache.org/jira/browse/MESOS-8680
Repository: mesos
Description (updated)
-------
Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
Diffs (updated)
-----
src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
Diff: https://reviews.apache.org/r/67567/diff/2/
Changes: https://reviews.apache.org/r/67567/diff/1-2/
Testing
-------
Thanks,
bin zheng
Re: Review Request 67567: Rename variable names in slave.hpp to be
more explicit.
Posted by bin zheng <zh...@chinaunicom.cn>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/
-----------------------------------------------------------
(Updated 六月 13, 2018, 5:14 p.m.)
Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
Bugs: MESOS-8680
https://issues.apache.org/jira/browse/MESOS-8680
Repository: mesos
Description
-------
Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
Diffs
-----
src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
Diff: https://reviews.apache.org/r/67567/diff/1/
Testing
-------
Thanks,
bin zheng
Re: Review Request 67567: Rename variable names in slave.hpp to be
more explicit.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67567/#review204707
-----------------------------------------------------------
Could you add the `bug`# to this patch, eg., `MESOS-XXXX`
and add the shepherd of this JIRA to the reviewer list.
- Gilbert Song
On June 12, 2018, 11:17 p.m., bin zheng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67567/
> -----------------------------------------------------------
>
> (Updated June 12, 2018, 11:17 p.m.)
>
>
> Review request for mesos and Gilbert Song.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the variable name of the Class "Slave", "Executor" and "Framework" in Slave.hpp, eg: change "info" to "slaveInfo", "executorInfo" or "frameworkInfo". also change others in related files: http.cpp and slave.cpp.
>
>
> Diffs
> -----
>
> src/slave/http.cpp a6739e12e55431a84844c747e584ef6420694076
> src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29
> src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a
>
>
> Diff: https://reviews.apache.org/r/67567/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bin zheng
>
>