impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Date Wed, 17 Aug 2016 05:58:29 GMT
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

Mime
View raw message