Kathy Sun has posted comments on this change. Change subject: IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage" ......................................................................
Patch Set 2: (7 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" Done Line 10: Set metric_group as a parameter of AddDefaultUrlCallbacks > For a bug like this, the best commit messages often have the following stru That makes more sense, thx for teaching me that. 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 state Hmm... ok, but why to change it this way? Currently, I set the last parameter "MetricGroup* metric_group" as NULL by default. So I think this could handle /statestore and /catalog situation. Besides, there is no metrics_ as a member in catalog-main and statestore-main, So I have to create a new metric pointer pointing to null , and pass it in? Is this change only for testing? or to make it consistent? 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 ther May I ask should we check if exist first...? And if we need that, should we plan 1: create a similar FindChildGroup() member function, or plan 2: create another checkIfExist() and call checkIfExist() and GetChildGroup() together? plan 2 is more concise I think, but will find twice. PS2, Line 113: JvmGroup > jvm_group Done PS2, Line 134: if(metric_group == NULL){ : LOG(INFO)<< "METRIC_GROUP IS NULL!!!!!"; : } > Remove! Done 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 Yeah, I was thinking about it. But what is the difference between this two? -- 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
