Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201608170558.u7H5wTbj018302@ip-10-146-233-104.ec2.internal>
Date: Wed, 17 Aug 2016 05:58:29 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Kathy Sun <kathy.sun@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3981=3A_Fixed_the_bug_of_=22Crash_when_access_statestore/catalog_memz_webpage=22=0A?=
In-Reply-To: <gerrit.1471313237000.If59b10f20044d1a468f27810a3029fe18fb19f29@gerrit.cloudera.org>
References: <gerrit.1471313237000.If59b10f20044d1a468f27810a3029fe18fb19f29@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Wed, 17 Aug 2016 05:58:34 -0000

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3998/2//COMMIT_MSG
Commit Message:

PS2, Line 7: Fixed the bug of "Crash when access statestore/catalog
           : memz webpage"
"Fix crash when accessing statestore / catalogd /memz page"


Line 10: Set metric_group as a parameter of AddDefaultUrlCallbacks
For a bug like this, the best commit messages often have the following structure:

  1. Describe the bug, e.g. : "The /memz page tried to add JVM metrics even when they didn't exist for all daemons, not just Impala. This led to a crash when they tried to access ExecEnv::GetInstance() without an initialised ExecEnv".

  2. Describe the fix, e.g.: "To fix, changed the memz handler method to take an optional metric group, provided by the caller."

  3. Describe any other details, e.g.: "Used C++11 lambdas rather than boost::bind to help simplify the code" (but consider how important this detail is to the reader)

  4. Describe testing - as you have it is fine.


http://gerrit.cloudera.org:8080/#/c/3998/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS2, Line 428: AddDefaultUrlCallbacks
You should do this for the catalog page as well, and probably for the statestore (that will test whether your code works when there is no JVM group present).


http://gerrit.cloudera.org:8080/#/c/3998/2/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:

Line 113:     MetricGroup* JvmGroup = metric_group->GetChildGroup("jvm");
You should check if the JVM group actually exists first. Unfortunately there's no method to do that in MetricGroup, so please add one that looks like GetChildGroup() but does not create the group if it doesn't exist. Maybe call the new method FindChildGroup()?


PS2, Line 113: JvmGroup
jvm_group


PS2, Line 114: (JvmGroup != NULL)
this will never happen when using GetChildGroup(), see above comment.


PS2, Line 134: if(metric_group == NULL){
             :     LOG(INFO)<< "METRIC_GROUP IS NULL!!!!!";
             :   }
Remove!


http://gerrit.cloudera.org:8080/#/c/3998/2/be/src/util/default-path-handlers.h
File be/src/util/default-path-handlers.h:

Line 22: #include "util/metrics.h"
rather than including metrics.h, it's better to forward declare MetricGroup like the other classes on lines 26-27.


-- 
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes