Henry Robinson has posted comments on this change. Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/3998/4/be/src/catalog/catalogd-main.cc File be/src/catalog/catalogd-main.cc: PS4, Line 71: scoped_ptr<MetricGroup> metrics(new MetricGroup("catalog")); This is the metric group you should pass in to AddDefaulUrlCallbacks(). Same in statestored-main.cc. http://gerrit.cloudera.org:8080/#/c/3998/4/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: PS4, Line 112: Value jvm(kObjectType); move this inside the if statement, to line 115. PS4, Line 113: GetChildGroup FindChildGroup(), right? http://gerrit.cloudera.org:8080/#/c/3998/4/be/src/util/default-path-handlers.h File be/src/util/default-path-handlers.h: PS4, Line 27: class MetricGroup; the reason we do this is that it removes a compilation dependency between this file and metrics.h. Before, if you changed metrics.h, you would have to recompile every file that includes this header. But because you're only using pointers, the compiler only needs to know the name of the class to compile the function declaration below. http://gerrit.cloudera.org:8080/#/c/3998/4/be/src/util/metrics.cc File be/src/util/metrics.cc: Line 210: else return NULL; remove 'else' http://gerrit.cloudera.org:8080/#/c/3998/4/be/src/util/metrics.h File be/src/util/metrics.h: PS4, Line 302: GetChildGroup Can you rename this to GetOrCreateChildGroup() now? PS4, Line 304: ; Better to say something like "Returns a child metric group with name 'name', or NULL if that group doesn't exist." -- 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: 4 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
