mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From haosdent <haosd...@gmail.com>
Subject Re: mesos git commit: Replaced CHECK with CHECK_READY.
Date Sun, 08 May 2016 03:48:00 GMT
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 <bmahler@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, <alexr@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 <neil.conway@gmail.com>
> > Authored: Mon May 2 13:05:56 2016 +0200
> > Committer: Alexander Rukletsov <alexr@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message