You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/01/24 14:54:01 UTC
Review Request 55876: Avoided shadowing in `Slave::run()`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
Diff: https://reviews.apache.org/r/55876/diff/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/#review162824
-----------------------------------------------------------
Ship it!
Ship It!
- Gast�n Kleiman
On Jan. 24, 2017, 2:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
>
> Diff: https://reviews.apache.org/r/55876/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 27, 2017, 5:17 p.m., Neil Conway wrote:
> > src/slave/slave.cpp, line 1580
> > <https://reviews.apache.org/r/55876/diff/3/?file=1617529#file1617529line1580>
> >
> > If we're going to change this, there are a few other places that should also be changed: e.g., lines 1179, 1226, 1580, potentially 5292.
Good point. I'm inclined to do it in a separate patch then.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/#review163288
-----------------------------------------------------------
On Jan. 27, 2017, 11:52 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 11:52 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
>
> Diff: https://reviews.apache.org/r/55876/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/#review163288
-----------------------------------------------------------
Fix it, then Ship it!
Variable naming still seems inconsistent, but I see we use both `var_` and `_var` in `slave.cpp`. Would be nice to be consistent but we can defer that to later.
src/slave/slave.cpp (line 1580)
<https://reviews.apache.org/r/55876/#comment234720>
If we're going to change this, there are a few other places that should also be changed: e.g., lines 1179, 1226, 1580, potentially 5292.
- Neil Conway
On Jan. 27, 2017, 11:52 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 11:52 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
>
> Diff: https://reviews.apache.org/r/55876/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/#review163261
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55876]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 27, 2017, 11:52 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 11:52 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
>
> Diff: https://reviews.apache.org/r/55876/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/
-----------------------------------------------------------
(Updated Jan. 27, 2017, 11:52 a.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
Diff: https://reviews.apache.org/r/55876/diff/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/
-----------------------------------------------------------
(Updated Jan. 27, 2017, 11:39 a.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
Diff: https://reviews.apache.org/r/55876/diff/
Testing
-------
make check
Thanks,
Alexander Rukletsov
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Jan. 24, 2017, 6:14 p.m., Neil Conway wrote:
> > src/slave/slave.cpp, line 1579
> > <https://reviews.apache.org/r/55876/diff/1/?file=1613399#file1613399line1579>
> >
> > I feel like we use an underscore suffix (`task_`) more often than a prefix. Is there a rule here?
From the naming section of our style guide:
```
We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing:
Prefer trailing underscores for use as member fields (but not required). Some trailing underscores are used to distinguish between similar variables in the same scope (think prime symbols), but this should be avoided as much as possible, including removing existing instances in the code base.
```
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/#review162836
-----------------------------------------------------------
On Jan. 24, 2017, 2:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
>
> Diff: https://reviews.apache.org/r/55876/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 55876: Avoided shadowing in `Slave::run()`.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55876/#review162836
-----------------------------------------------------------
src/slave/slave.cpp (line 1579)
<https://reviews.apache.org/r/55876/#comment234202>
I feel like we use an underscore suffix (`task_`) more often than a prefix. Is there a rule here?
src/slave/slave.cpp (line 1580)
<https://reviews.apache.org/r/55876/#comment234203>
Not yours, but is there a reason why this isn't written `if (task.slave_id() != info.id()) {...}` ?
src/slave/slave.cpp (line 1679)
<https://reviews.apache.org/r/55876/#comment234201>
Also variable shadowing.
- Neil Conway
On Jan. 24, 2017, 2:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55876/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 2:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891
>
> Diff: https://reviews.apache.org/r/55876/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rukletsov
>
>