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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Kathy Sun <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
