----------------------------------------------------------- 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 > >
