You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2016/05/08 00:38:58 UTC
Re: mesos git commit: Replaced CHECK with CHECK_READY.
Hm.. any reason that unrelated headers were touched and the using statement
was removed in this patch?
My concern with mixing unrelated changes within a single patch is that
patches become less precise. If one reads the patch there is additional
overhead in understanding what is related to the goal of the change and
what is not. I know it's a small example here but I see value in being
disciplined about this regardless of patch size.
The other concern is that the reviewer of this patch has to review these
two additional changes:
1. How does the header audit look? Anything else need added or removed?
2. How does the 'using' audit look? Anything else need added or removed?
(1) and (2) could be done together in a single patch. As in turns out, the
header audit looks like it has a few issues, but I'm guessing the reviewer
glossed over it because the point of this change was CHECK_READY :)
-<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used
-<glog/logging.h> is not present but LOG is used
-<stout/nothing.hpp> is absent but Nothing is used
-<process/process.hpp> is absent but process::Process / process::wait /
process::terminate are used.
Then for the 'using' audit, we now avoid pulling in all of the process::
namespace in favor of finer-grained using statements.
On Mon, May 2, 2016 at 4:08 AM, <al...@apache.org> wrote:
> Repository: mesos
> Updated Branches:
> refs/heads/master 78f6101cc -> 4f9040db6
>
>
> Replaced CHECK with CHECK_READY.
>
> Also removes some unused header includes.
>
> Review: https://reviews.apache.org/r/46827/
>
>
> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db
> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db
> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db
>
> Branch: refs/heads/master
> Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310
> Parents: 78f6101
> Author: Neil Conway <ne...@gmail.com>
> Authored: Mon May 2 13:05:56 2016 +0200
> Committer: Alexander Rukletsov <al...@apache.org>
> Committed: Mon May 2 13:05:56 2016 +0200
>
> ----------------------------------------------------------------------
> src/zookeeper/contender.cpp | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
> ----------------------------------------------------------------------
>
>
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp
> ----------------------------------------------------------------------
> diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp
> index 4b1cc65..5eb4b58 100644
> --- a/src/zookeeper/contender.cpp
> +++ b/src/zookeeper/contender.cpp
> @@ -14,26 +14,23 @@
> // See the License for the specific language governing permissions and
> // limitations under the License
>
> -#include <set>
> #include <string>
>
> #include <mesos/zookeeper/contender.hpp>
> #include <mesos/zookeeper/detector.hpp>
> #include <mesos/zookeeper/group.hpp>
>
> +#include <process/check.hpp>
> #include <process/defer.hpp>
> #include <process/dispatch.hpp>
> #include <process/future.hpp>
> #include <process/id.hpp>
>
> -#include <stout/check.hpp>
> #include <stout/lambda.hpp>
> #include <stout/option.hpp>
> -#include <stout/some.hpp>
>
> using namespace process;
>
> -using std::set;
> using std::string;
>
> namespace zookeeper {
> @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel()
>
> void LeaderContenderProcess::cancelled(const Future<bool>& result)
> {
> - CHECK(candidacy.isReady());
> + CHECK_READY(candidacy);
> LOG(INFO) << "Membership cancelled: " << candidacy.get().id();
>
> // Can be called as a result of either withdraw() or server side
>
>
Re: mesos git commit: Replaced CHECK with CHECK_READY.
Posted by Cong Wang <cw...@twopensource.com>.
On Sun, May 8, 2016 at 1:28 AM, Alex R <al...@apache.org> wrote:
> I agree that "atomic patches" (those that do one thing per patch) are a
> good thing because they simplify navigating history, do blame and bisect.
> But how to define that "one thing"? Some people would say, that a new
If "one thing" can't be defined with regarding to a specific patch,
then people don't understand the code, period.
> feature is one thing, and if introducing a feature requires some
> refactoring, it should be done in the same patch, so that motivation for
> the refactoring is clear. On the other hand there will be folks who would
That doesn't make sense, refactoring can almost always be done in a
separated patch, otherwise it would hurt readability. If they are _logically_
separated, they should be separated.
> say that putting all loosely-coupled changes like refactoring, tests,
> protobuf update, implementation into the same patch makes it hardly
> reviewable and complicates finding problems via blame / bisect.
>
> Let's take a step back and think, why we need to read commit history in the
> first place. Based on my experience, I see several cases:
> * Searching which patch introduced a bug, a regression, or some peculiar
> behaviour.
> * Learning how a feature was implemented and why.
>
> That's why it is important to separate functional changes from cleanups.
>
> However, do we still want to separate different sorts of cleanups as well
> or squash them together to avoid the churn? If I search for a bug and see a
> style fix patch, I simply skip. I'd rather prefer to have one single patch
> for all style fixes than a tiny patch for each type of cleanup.
This is what Linux kernel has been doing for years. I know people here
don't like to be compared with Linux kernel, but you need to realize that
it is the best practice.
>
> Regarding r/46827/ <https://reviews.apache.org/r/46827/>, you are right
> that doing audit of include and using sections was not the primary
> intention, but I considered it fine to include those changes since they
> were not making the code "worse" (modulo removing <stout/check.hpp> which
> was a mistake).
No one intends to make things worse.
It is not a personal taste or something, you have to make it a rule,
otherwise same mistake would repeat.
Re: mesos git commit: Replaced CHECK with CHECK_READY.
Posted by Benjamin Mahler <bm...@apache.org>.
That makes sense, if the summary of the change was "Style cleanups within
..." rather than "Replaced CHECK with CHECK_READY" then doing an overall
style sweep of the file together sounds ok to me. Here the reviewer will be
more likely to look through the file for other style changes that are
needed (e.g. s/.get()./->/, using statements, etc).
When I read the CHECK_READY summary I tend to expect an isolated change
addressing CHECK_READY, ideally a complete (rather than one file) sweep to
use the CHECK_READY pattern in the code base.
Thanks for cleaning this stuff up Neil!
On Tue, May 10, 2016 at 6:25 AM, Neil Conway <ne...@gmail.com> wrote:
> Hi Ben,
>
> Thanks for raising this!
>
> My thinking for grouping the changes together in a single review is
> basically what AlexR said: I agree with doing "one thing per patch",
> but I felt like a header cleanup was sufficiently close to CHECK
> cleanup that they could be grouped together. If there's consensus that
> folks would rather see such changes separated, I'm happy to do that in
> the future.
>
> Re: the <stout/check.hpp> change, my mistake -- although I confess I
> didn't realize we have a strict "no dependence on transitive includes"
> policy. Is that documented anywhere?
>
> Anyway, RRs here:
>
> https://reviews.apache.org/r/47112 avoids depending on transitively
> included headers
> https://reviews.apache.org/r/47113 replaces "using namespace process;"
> with more fine-grained "using" statements
> https://reviews.apache.org/r/47114 fixes angle bracket style.
>
> Thanks,
> Neil
>
> On Sun, May 8, 2016 at 10:28 AM, Alex R <al...@apache.org> wrote:
> > I agree that "atomic patches" (those that do one thing per patch) are a
> good
> > thing because they simplify navigating history, do blame and bisect. But
> how
> > to define that "one thing"? Some people would say, that a new feature is
> one
> > thing, and if introducing a feature requires some refactoring, it should
> be
> > done in the same patch, so that motivation for the refactoring is clear.
> On
> > the other hand there will be folks who would say that putting all
> > loosely-coupled changes like refactoring, tests, protobuf update,
> > implementation into the same patch makes it hardly reviewable and
> > complicates finding problems via blame / bisect.
> >
> > Let's take a step back and think, why we need to read commit history in
> the
> > first place. Based on my experience, I see several cases:
> > * Searching which patch introduced a bug, a regression, or some
> peculiar
> > behaviour.
> > * Learning how a feature was implemented and why.
> >
> > That's why it is important to separate functional changes from cleanups.
> >
> > However, do we still want to separate different sorts of cleanups as
> well or
> > squash them together to avoid the churn? If I search for a bug and see a
> > style fix patch, I simply skip. I'd rather prefer to have one single
> patch
> > for all style fixes than a tiny patch for each type of cleanup.
> >
> > Regarding r/46827/, you are right that doing audit of include and using
> > sections was not the primary intention, but I considered it fine to
> include
> > those changes since they were not making the code "worse" (modulo
> removing
> > <stout/check.hpp> which was a mistake).
> >
> > On 8 May 2016 at 02:38, Benjamin Mahler <bm...@apache.org> wrote:
> >>
> >> Hm.. any reason that unrelated headers were touched and the using
> >> statement was removed in this patch?
> >>
> >> My concern with mixing unrelated changes within a single patch is that
> >> patches become less precise. If one reads the patch there is additional
> >> overhead in understanding what is related to the goal of the change and
> what
> >> is not. I know it's a small example here but I see value in being
> >> disciplined about this regardless of patch size.
> >>
> >> The other concern is that the reviewer of this patch has to review these
> >> two additional changes:
> >> 1. How does the header audit look? Anything else need added or
> removed?
> >> 2. How does the 'using' audit look? Anything else need added or
> removed?
> >>
> >> (1) and (2) could be done together in a single patch. As in turns out,
> the
> >> header audit looks like it has a few issues, but I'm guessing the
> reviewer
> >> glossed over it because the point of this change was CHECK_READY :)
> >> -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used
> >> -<glog/logging.h> is not present but LOG is used
> >> -<stout/nothing.hpp> is absent but Nothing is used
> >> -<process/process.hpp> is absent but process::Process / process::wait
> /
> >> process::terminate are used.
> >>
> >> Then for the 'using' audit, we now avoid pulling in all of the process::
> >> namespace in favor of finer-grained using statements.
> >>
> >> On Mon, May 2, 2016 at 4:08 AM, <al...@apache.org> wrote:
> >>>
> >>> Repository: mesos
> >>> Updated Branches:
> >>> refs/heads/master 78f6101cc -> 4f9040db6
> >>>
> >>>
> >>> Replaced CHECK with CHECK_READY.
> >>>
> >>> Also removes some unused header includes.
> >>>
> >>> Review: https://reviews.apache.org/r/46827/
> >>>
> >>>
> >>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> >>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db
> >>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db
> >>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db
> >>>
> >>> Branch: refs/heads/master
> >>> Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310
> >>> Parents: 78f6101
> >>> Author: Neil Conway <ne...@gmail.com>
> >>> Authored: Mon May 2 13:05:56 2016 +0200
> >>> Committer: Alexander Rukletsov <al...@apache.org>
> >>> Committed: Mon May 2 13:05:56 2016 +0200
> >>>
> >>> ----------------------------------------------------------------------
> >>> src/zookeeper/contender.cpp | 7 ++-----
> >>> 1 file changed, 2 insertions(+), 5 deletions(-)
> >>> ----------------------------------------------------------------------
> >>>
> >>>
> >>>
> >>>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp
> >>> ----------------------------------------------------------------------
> >>> diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp
> >>> index 4b1cc65..5eb4b58 100644
> >>> --- a/src/zookeeper/contender.cpp
> >>> +++ b/src/zookeeper/contender.cpp
> >>> @@ -14,26 +14,23 @@
> >>> // See the License for the specific language governing permissions and
> >>> // limitations under the License
> >>>
> >>> -#include <set>
> >>> #include <string>
> >>>
> >>> #include <mesos/zookeeper/contender.hpp>
> >>> #include <mesos/zookeeper/detector.hpp>
> >>> #include <mesos/zookeeper/group.hpp>
> >>>
> >>> +#include <process/check.hpp>
> >>> #include <process/defer.hpp>
> >>> #include <process/dispatch.hpp>
> >>> #include <process/future.hpp>
> >>> #include <process/id.hpp>
> >>>
> >>> -#include <stout/check.hpp>
> >>> #include <stout/lambda.hpp>
> >>> #include <stout/option.hpp>
> >>> -#include <stout/some.hpp>
> >>>
> >>> using namespace process;
> >>>
> >>> -using std::set;
> >>> using std::string;
> >>>
> >>> namespace zookeeper {
> >>> @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel()
> >>>
> >>> void LeaderContenderProcess::cancelled(const Future<bool>& result)
> >>> {
> >>> - CHECK(candidacy.isReady());
> >>> + CHECK_READY(candidacy);
> >>> LOG(INFO) << "Membership cancelled: " << candidacy.get().id();
> >>>
> >>> // Can be called as a result of either withdraw() or server side
> >>>
> >>
> >
>
Re: mesos git commit: Replaced CHECK with CHECK_READY.
Posted by Neil Conway <ne...@gmail.com>.
Hi Ben,
Thanks for raising this!
My thinking for grouping the changes together in a single review is
basically what AlexR said: I agree with doing "one thing per patch",
but I felt like a header cleanup was sufficiently close to CHECK
cleanup that they could be grouped together. If there's consensus that
folks would rather see such changes separated, I'm happy to do that in
the future.
Re: the <stout/check.hpp> change, my mistake -- although I confess I
didn't realize we have a strict "no dependence on transitive includes"
policy. Is that documented anywhere?
Anyway, RRs here:
https://reviews.apache.org/r/47112 avoids depending on transitively
included headers
https://reviews.apache.org/r/47113 replaces "using namespace process;"
with more fine-grained "using" statements
https://reviews.apache.org/r/47114 fixes angle bracket style.
Thanks,
Neil
On Sun, May 8, 2016 at 10:28 AM, Alex R <al...@apache.org> wrote:
> I agree that "atomic patches" (those that do one thing per patch) are a good
> thing because they simplify navigating history, do blame and bisect. But how
> to define that "one thing"? Some people would say, that a new feature is one
> thing, and if introducing a feature requires some refactoring, it should be
> done in the same patch, so that motivation for the refactoring is clear. On
> the other hand there will be folks who would say that putting all
> loosely-coupled changes like refactoring, tests, protobuf update,
> implementation into the same patch makes it hardly reviewable and
> complicates finding problems via blame / bisect.
>
> Let's take a step back and think, why we need to read commit history in the
> first place. Based on my experience, I see several cases:
> * Searching which patch introduced a bug, a regression, or some peculiar
> behaviour.
> * Learning how a feature was implemented and why.
>
> That's why it is important to separate functional changes from cleanups.
>
> However, do we still want to separate different sorts of cleanups as well or
> squash them together to avoid the churn? If I search for a bug and see a
> style fix patch, I simply skip. I'd rather prefer to have one single patch
> for all style fixes than a tiny patch for each type of cleanup.
>
> Regarding r/46827/, you are right that doing audit of include and using
> sections was not the primary intention, but I considered it fine to include
> those changes since they were not making the code "worse" (modulo removing
> <stout/check.hpp> which was a mistake).
>
> On 8 May 2016 at 02:38, Benjamin Mahler <bm...@apache.org> wrote:
>>
>> Hm.. any reason that unrelated headers were touched and the using
>> statement was removed in this patch?
>>
>> My concern with mixing unrelated changes within a single patch is that
>> patches become less precise. If one reads the patch there is additional
>> overhead in understanding what is related to the goal of the change and what
>> is not. I know it's a small example here but I see value in being
>> disciplined about this regardless of patch size.
>>
>> The other concern is that the reviewer of this patch has to review these
>> two additional changes:
>> 1. How does the header audit look? Anything else need added or removed?
>> 2. How does the 'using' audit look? Anything else need added or removed?
>>
>> (1) and (2) could be done together in a single patch. As in turns out, the
>> header audit looks like it has a few issues, but I'm guessing the reviewer
>> glossed over it because the point of this change was CHECK_READY :)
>> -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used
>> -<glog/logging.h> is not present but LOG is used
>> -<stout/nothing.hpp> is absent but Nothing is used
>> -<process/process.hpp> is absent but process::Process / process::wait /
>> process::terminate are used.
>>
>> Then for the 'using' audit, we now avoid pulling in all of the process::
>> namespace in favor of finer-grained using statements.
>>
>> On Mon, May 2, 2016 at 4:08 AM, <al...@apache.org> wrote:
>>>
>>> Repository: mesos
>>> Updated Branches:
>>> refs/heads/master 78f6101cc -> 4f9040db6
>>>
>>>
>>> Replaced CHECK with CHECK_READY.
>>>
>>> Also removes some unused header includes.
>>>
>>> Review: https://reviews.apache.org/r/46827/
>>>
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db
>>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db
>>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db
>>>
>>> Branch: refs/heads/master
>>> Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310
>>> Parents: 78f6101
>>> Author: Neil Conway <ne...@gmail.com>
>>> Authored: Mon May 2 13:05:56 2016 +0200
>>> Committer: Alexander Rukletsov <al...@apache.org>
>>> Committed: Mon May 2 13:05:56 2016 +0200
>>>
>>> ----------------------------------------------------------------------
>>> src/zookeeper/contender.cpp | 7 ++-----
>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>> ----------------------------------------------------------------------
>>>
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp
>>> ----------------------------------------------------------------------
>>> diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp
>>> index 4b1cc65..5eb4b58 100644
>>> --- a/src/zookeeper/contender.cpp
>>> +++ b/src/zookeeper/contender.cpp
>>> @@ -14,26 +14,23 @@
>>> // See the License for the specific language governing permissions and
>>> // limitations under the License
>>>
>>> -#include <set>
>>> #include <string>
>>>
>>> #include <mesos/zookeeper/contender.hpp>
>>> #include <mesos/zookeeper/detector.hpp>
>>> #include <mesos/zookeeper/group.hpp>
>>>
>>> +#include <process/check.hpp>
>>> #include <process/defer.hpp>
>>> #include <process/dispatch.hpp>
>>> #include <process/future.hpp>
>>> #include <process/id.hpp>
>>>
>>> -#include <stout/check.hpp>
>>> #include <stout/lambda.hpp>
>>> #include <stout/option.hpp>
>>> -#include <stout/some.hpp>
>>>
>>> using namespace process;
>>>
>>> -using std::set;
>>> using std::string;
>>>
>>> namespace zookeeper {
>>> @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel()
>>>
>>> void LeaderContenderProcess::cancelled(const Future<bool>& result)
>>> {
>>> - CHECK(candidacy.isReady());
>>> + CHECK_READY(candidacy);
>>> LOG(INFO) << "Membership cancelled: " << candidacy.get().id();
>>>
>>> // Can be called as a result of either withdraw() or server side
>>>
>>
>
Re: mesos git commit: Replaced CHECK with CHECK_READY.
Posted by Alex R <al...@apache.org>.
I agree that "atomic patches" (those that do one thing per patch) are a
good thing because they simplify navigating history, do blame and bisect.
But how to define that "one thing"? Some people would say, that a new
feature is one thing, and if introducing a feature requires some
refactoring, it should be done in the same patch, so that motivation for
the refactoring is clear. On the other hand there will be folks who would
say that putting all loosely-coupled changes like refactoring, tests,
protobuf update, implementation into the same patch makes it hardly
reviewable and complicates finding problems via blame / bisect.
Let's take a step back and think, why we need to read commit history in the
first place. Based on my experience, I see several cases:
* Searching which patch introduced a bug, a regression, or some peculiar
behaviour.
* Learning how a feature was implemented and why.
That's why it is important to separate functional changes from cleanups.
However, do we still want to separate different sorts of cleanups as well
or squash them together to avoid the churn? If I search for a bug and see a
style fix patch, I simply skip. I'd rather prefer to have one single patch
for all style fixes than a tiny patch for each type of cleanup.
Regarding r/46827/ <https://reviews.apache.org/r/46827/>, you are right
that doing audit of include and using sections was not the primary
intention, but I considered it fine to include those changes since they
were not making the code "worse" (modulo removing <stout/check.hpp> which
was a mistake).
On 8 May 2016 at 02:38, Benjamin Mahler <bm...@apache.org> wrote:
> Hm.. any reason that unrelated headers were touched and the using
> statement was removed in this patch?
>
> My concern with mixing unrelated changes within a single patch is that
> patches become less precise. If one reads the patch there is additional
> overhead in understanding what is related to the goal of the change and
> what is not. I know it's a small example here but I see value in being
> disciplined about this regardless of patch size.
>
> The other concern is that the reviewer of this patch has to review these
> two additional changes:
> 1. How does the header audit look? Anything else need added or removed?
> 2. How does the 'using' audit look? Anything else need added or removed?
>
> (1) and (2) could be done together in a single patch. As in turns out, the
> header audit looks like it has a few issues, but I'm guessing the reviewer
> glossed over it because the point of this change was CHECK_READY :)
> -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used
> -<glog/logging.h> is not present but LOG is used
> -<stout/nothing.hpp> is absent but Nothing is used
> -<process/process.hpp> is absent but process::Process / process::wait /
> process::terminate are used.
>
> Then for the 'using' audit, we now avoid pulling in all of the process::
> namespace in favor of finer-grained using statements.
>
> On Mon, May 2, 2016 at 4:08 AM, <al...@apache.org> wrote:
>
>> Repository: mesos
>> Updated Branches:
>> refs/heads/master 78f6101cc -> 4f9040db6
>>
>>
>> Replaced CHECK with CHECK_READY.
>>
>> Also removes some unused header includes.
>>
>> Review: https://reviews.apache.org/r/46827/
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db
>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db
>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db
>>
>> Branch: refs/heads/master
>> Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310
>> Parents: 78f6101
>> Author: Neil Conway <ne...@gmail.com>
>> Authored: Mon May 2 13:05:56 2016 +0200
>> Committer: Alexander Rukletsov <al...@apache.org>
>> Committed: Mon May 2 13:05:56 2016 +0200
>>
>> ----------------------------------------------------------------------
>> src/zookeeper/contender.cpp | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp
>> ----------------------------------------------------------------------
>> diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp
>> index 4b1cc65..5eb4b58 100644
>> --- a/src/zookeeper/contender.cpp
>> +++ b/src/zookeeper/contender.cpp
>> @@ -14,26 +14,23 @@
>> // See the License for the specific language governing permissions and
>> // limitations under the License
>>
>> -#include <set>
>> #include <string>
>>
>> #include <mesos/zookeeper/contender.hpp>
>> #include <mesos/zookeeper/detector.hpp>
>> #include <mesos/zookeeper/group.hpp>
>>
>> +#include <process/check.hpp>
>> #include <process/defer.hpp>
>> #include <process/dispatch.hpp>
>> #include <process/future.hpp>
>> #include <process/id.hpp>
>>
>> -#include <stout/check.hpp>
>> #include <stout/lambda.hpp>
>> #include <stout/option.hpp>
>> -#include <stout/some.hpp>
>>
>> using namespace process;
>>
>> -using std::set;
>> using std::string;
>>
>> namespace zookeeper {
>> @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel()
>>
>> void LeaderContenderProcess::cancelled(const Future<bool>& result)
>> {
>> - CHECK(candidacy.isReady());
>> + CHECK_READY(candidacy);
>> LOG(INFO) << "Membership cancelled: " << candidacy.get().id();
>>
>> // Can be called as a result of either withdraw() or server side
>>
>>
>
Re: mesos git commit: Replaced CHECK with CHECK_READY.
Posted by haosdent <ha...@gmail.com>.
I notice we sweep update the code which the style is incorrect. For
example, replace "A<<B> >" with "A<<B>>", replace ".get()" to "->", remove
incorrect space, adjust line length. Does this mean we need to split them
out as an additional patch?
On Sun, May 8, 2016 at 8:38 AM, Benjamin Mahler <bm...@apache.org> wrote:
> Hm.. any reason that unrelated headers were touched and the using statement
> was removed in this patch?
>
> My concern with mixing unrelated changes within a single patch is that
> patches become less precise. If one reads the patch there is additional
> overhead in understanding what is related to the goal of the change and
> what is not. I know it's a small example here but I see value in being
> disciplined about this regardless of patch size.
>
> The other concern is that the reviewer of this patch has to review these
> two additional changes:
> 1. How does the header audit look? Anything else need added or removed?
> 2. How does the 'using' audit look? Anything else need added or removed?
>
> (1) and (2) could be done together in a single patch. As in turns out, the
> header audit looks like it has a few issues, but I'm guessing the reviewer
> glossed over it because the point of this change was CHECK_READY :)
> -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used
> -<glog/logging.h> is not present but LOG is used
> -<stout/nothing.hpp> is absent but Nothing is used
> -<process/process.hpp> is absent but process::Process / process::wait /
> process::terminate are used.
>
> Then for the 'using' audit, we now avoid pulling in all of the process::
> namespace in favor of finer-grained using statements.
>
> On Mon, May 2, 2016 at 4:08 AM, <al...@apache.org> wrote:
>
> > Repository: mesos
> > Updated Branches:
> > refs/heads/master 78f6101cc -> 4f9040db6
> >
> >
> > Replaced CHECK with CHECK_READY.
> >
> > Also removes some unused header includes.
> >
> > Review: https://reviews.apache.org/r/46827/
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db
> > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db
> > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db
> >
> > Branch: refs/heads/master
> > Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310
> > Parents: 78f6101
> > Author: Neil Conway <ne...@gmail.com>
> > Authored: Mon May 2 13:05:56 2016 +0200
> > Committer: Alexander Rukletsov <al...@apache.org>
> > Committed: Mon May 2 13:05:56 2016 +0200
> >
> > ----------------------------------------------------------------------
> > src/zookeeper/contender.cpp | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp
> > ----------------------------------------------------------------------
> > diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp
> > index 4b1cc65..5eb4b58 100644
> > --- a/src/zookeeper/contender.cpp
> > +++ b/src/zookeeper/contender.cpp
> > @@ -14,26 +14,23 @@
> > // See the License for the specific language governing permissions and
> > // limitations under the License
> >
> > -#include <set>
> > #include <string>
> >
> > #include <mesos/zookeeper/contender.hpp>
> > #include <mesos/zookeeper/detector.hpp>
> > #include <mesos/zookeeper/group.hpp>
> >
> > +#include <process/check.hpp>
> > #include <process/defer.hpp>
> > #include <process/dispatch.hpp>
> > #include <process/future.hpp>
> > #include <process/id.hpp>
> >
> > -#include <stout/check.hpp>
> > #include <stout/lambda.hpp>
> > #include <stout/option.hpp>
> > -#include <stout/some.hpp>
> >
> > using namespace process;
> >
> > -using std::set;
> > using std::string;
> >
> > namespace zookeeper {
> > @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel()
> >
> > void LeaderContenderProcess::cancelled(const Future<bool>& result)
> > {
> > - CHECK(candidacy.isReady());
> > + CHECK_READY(candidacy);
> > LOG(INFO) << "Membership cancelled: " << candidacy.get().id();
> >
> > // Can be called as a result of either withdraw() or server side
> >
> >
>
--
Best Regards,
Haosdent Huang