You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Kathy Sun (Code Review)" <ge...@cloudera.org> on 2016/08/16 02:07:17 UTC
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Kathy Sun has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3998
Change subject: IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
......................................................................
IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog
memz webpage"
Add ExecEnv instance not NULL check
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/util/default-path-handlers.cc
M www/memz.tmpl
2 files changed, 11 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/1
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#8).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Add a test file test_web_pages.py to test sending request to /memz on
impalad / statestored / catalogd
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/memory-metrics.cc
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
A tests/webserver/test_web_pages.py
M www/memz.tmpl
15 files changed, 96 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/8
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#3).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/runtime/exec-env.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M www/memz.tmpl
4 files changed, 32 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/3
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 10: Verified+1
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun 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(). Sam
Done
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.
Done
PS4, Line 113: GetChildGroup
> FindChildGroup(), right?
Done
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 t
I see. It's my first time to know that. THX!
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'
Done
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?
Done
PS4, Line 304: ;
> Better to say something like "Returns a child metric group with name 'name'
Done
--
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 <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3998/7/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS7, Line 17: #
: # Verification of impalad metrics after a test run.
remove
PS7, Line 21: from tests.common.errors import Timeout
is this used?
PS7, Line 27: (\
remove the \, and try and put some of the parameters onto this line.
PS7, Line 31: resp.raise_for_status()
does this raise an error when status != 200? If so you don't need the checks on lines 41, 43, 45. Maybe you could check if the contents of the webpage has what you're expecting?
PS7, Line 35: """request web page /memz at imapalad / statestored / catalogd."""
I'd remove this, it doesn't tell the reader anything that's not in the test comments.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
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 1:
I think we can do something a bit cleaner, after thinking about it.
Everywhere that AddDefaultPathHandler() is called, there should be a MetricGroup* object that is the top-level metric group for the process. You can pass that into AddDefaultPathHandler() just like the process mem tracker, and use that to get at the JVM metrics - if they exist.
To do that you should rewrite
webserver->RegisterUrlCallback("/memz","memz.tmpl",
bind<void>(&MemUsageHandler, process_mem_tracker, _1, _2));
to something like
auto callback = [metric_group, process_mem_tracker](const auto& args, auto* doc) {
MemUsageHandler(process_mem_tracker, metric_group, args, doc);
}
webserver->RegisterUrlCallback("/memz", callback);
(This is using C++11's new lambda syntax which I find much cleaner than boost::bind).
You should deal with the cases where a) metric_group == NULL and b) there is no "jvm" subgroup in the metric group.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#5).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/memory-metrics.cc
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
M www/memz.tmpl
14 files changed, 55 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/5
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3998
to look at the new patch set (#9).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Add a test file test_web_pages.py to test sending request to /memz on
impalad / statestored / catalogd
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/memory-metrics.cc
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
A tests/webserver/test_web_pages.py
M www/memz.tmpl
15 files changed, 96 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/9
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
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 <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 8: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3998/8/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS8, Line 23: imapalad
impalad
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
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:
I will add test of webpage in next patch
--
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 <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
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 <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#4).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/exec-env.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/metrics.cc
M be/src/util/metrics.h
M www/memz.tmpl
7 files changed, 41 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/4
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 6:
(8 comments)
Glad to see the test!
http://gerrit.cloudera.org:8080/#/c/3998/6/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
PS6, Line 349:
single space only
PS6, Line 350: reinterpret_cast<MetricGroup*>(NULL)
just wondering - does nullptr work here?
http://gerrit.cloudera.org:8080/#/c/3998/6/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS6, Line 27: webinfo
should be:
class Webinfo(object):
- but I think we should remove the class.
PS6, Line 37: raise Timeout(underlying_exception=e)
if you raise here, line 38 will never be executed. I think you don't need the try / except blocks, unless there's some specific error condition you want to deal with here.
PS6, Line 27: class webinfo:
: def __init__(self):
: self.host_name = "localhost"
: self.web_ui_port = None
:
: def _request_web_page(self, relative_url, params={}, timeout_secs=DEFAULT_TIMEOUT):
: url = "http://%s:%s%s" % (self.host_name, self.web_ui_port, relative_url)
: try:
: resp = requests.get(url, params=params, timeout=timeout_secs)
: except requests.exceptions.Timeout as e:
: raise Timeout(underlying_exception=e)
: resp.raise_for_status()
: return resp
Don't think you need a class for this. Just have:
def request_web_page(...):
you can pass the port in yourself.
PS6, Line 44: "Test that all the memz/"
comment needs finishing
PS6, Line 45: relative_url = "/memz"
I feel like this test can be written like:
for host, port in (("localhost", 25000), ("localhost", 25010), ("localhost", 25020):
page = request_web_page(host, port, "/memz")
# assert whatever you need to about 'page' here
PS6, Line 49: impalad._request_web_page(relative_url)
Does this fail if the web page 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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#2).
Change subject: IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
......................................................................
IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog
memz webpage"
Set metric_group as a parameter of AddDefaultUrlCallbacks
Change binding with Lambda function to symplify the code
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/runtime/exec-env.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M www/memz.tmpl
4 files changed, 32 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/2
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3998/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
Line 326: ImpaladMetrics::CreateMetrics(
> long line
Done
http://gerrit.cloudera.org:8080/#/c/3998/5/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:
PS5, Line 117: ++i
> revert this to ++i please
Done
http://gerrit.cloudera.org:8080/#/c/3998/5/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
Line 228:
> Can you add a quick test for FindChildGroup?
Done
http://gerrit.cloudera.org:8080/#/c/3998/5/www/memz.tmpl
File www/memz.tmpl:
Line 60:
> nit: remove blank line
Done
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
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 <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 5:
(4 comments)
Looks good. How about that webpage test?
http://gerrit.cloudera.org:8080/#/c/3998/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
Line 326: ImpaladMetrics::CreateMetrics(exec_env->metrics()->GetOrCreateChildGroup("impala-server"));
long line
http://gerrit.cloudera.org:8080/#/c/3998/5/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:
PS5, Line 117: i++
revert this to ++i please
http://gerrit.cloudera.org:8080/#/c/3998/5/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
Line 228:
Can you add a quick test for FindChildGroup?
http://gerrit.cloudera.org:8080/#/c/3998/5/www/memz.tmpl
File www/memz.tmpl:
Line 60:
nit: remove blank line
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 7:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/3998/6/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:
PS6, Line 350: reinterpret_cast<MetricGroup*>(NULL)
> just wondering - does nullptr work here?
Yeah, It could work, I write in this way since lines 372 did so.
to make them consistent
http://gerrit.cloudera.org:8080/#/c/3998/6/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS6, Line 27: quest_w
> should be:
Done
PS6, Line 37: "Test all memz/ webpages"""
> if you raise here, line 38 will never be executed. I think you don't need t
Done
PS6, Line 27: def request_web_page(\
: host_name, web_ui_port, relative_url, params={}, timeout_secs=DEFAULT_TIMEOUT):
: url = "http://%s:%s%s" % (host_name, web_ui_port, relative_url)
: resp = requests.get(url, params=params, timeout=timeout_secs)
: resp.raise_for_status()
: return resp
:
: class TestWebPage(ImpalaTestSuite):
: """request web page /memz at imapalad / statestored / catalogd."""
: def test_memz(self):
: """Test all memz/ webpages"""
: relative_url = "/memz"
:
> Don't think you need a class for this. Just have:
Done
PS6, Line 44: ge = request_web_page("lo
> comment needs finishing
Done
PS6, Line 45: assert page.status_cod
> I feel like this test can be written like:
Done
PS6, Line 49:
> Does this fail if the web page doesn't exist?
I add assert for the status_code, so it will fail now
http://gerrit.cloudera.org:8080/#/c/3998/7/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS7, Line 17: #
: # Verification of impalad metrics after a test run.
> remove
Done
PS7, Line 21: from tests.common.errors import Timeout
> is this used?
no, removed
PS7, Line 21: from tests.common.errors import Timeout
> is this used?
Done
PS7, Line 27: (\
> remove the \, and try and put some of the parameters onto this line.
I decided not to wrap requests.get() into another function
remove request_web_page() for cleaner code
PS7, Line 31: resp.raise_for_status()
> does this raise an error when status != 200? If so you don't need the check
I think status_code is enough for our purpose of checking whether the system crashed on clicking /memz.
PS7, Line 35: """request web page /memz at imapalad / statestored / catalogd."""
> I'd remove this, it doesn't tell the reader anything that's not in the test
Done
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Add a test file test_web_pages.py to test sending request to /memz on
impalad / statestored / catalogd
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Reviewed-on: http://gerrit.cloudera.org:8080/3998
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/memory-metrics.cc
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
A tests/webserver/test_web_pages.py
M www/memz.tmpl
15 files changed, 96 insertions(+), 31 deletions(-)
Approvals:
Internal Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#6).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Add a test file test_web_pages.py to test sending request to /memz on
impalad / statestored / catalogd
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/memory-metrics.cc
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
A tests/webserver/test_web_pages.py
M www/memz.tmpl
15 files changed, 123 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/6
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 9: Verified-1
Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/108/
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3981: Fixed the bug of "Crash when access statestore/catalog memz webpage"
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3998/1/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:
Line 112:
No need to add the extra line.
Line 114: if(ExecEnv::GetInstance()!= NULL){
Need extra whitespace.
if (ExecEnv::GetInstance() != NULL) {
Line 115: ExecEnv::GetInstance()->metrics()->GetChildGroup("jvm")->ToJson(false, document, &jvm);
Long line - wrap it before &jvm
Line 117: for(SizeType i = 0; i < jvm["metrics"].Size(); ++i){
Missing spaces here from before the change:
for (SizeType i = 0; i < jvm["metrics"].Size(); ++i) {
Line 118: if (strstr(jvm["metrics"][i]["name"].GetString(), "total") != nullptr){
Missing space before {
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 3:
(3 comments)
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
> Hmm... ok, but why to change it this way?
At patch3 I change the catalogd as you described temporarily, is this same as what you think?
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");
> May I ask should we check if exist first...?
Forget my two plans here.. I was being silly. Now I get it.
PS2, Line 114: (JvmGroup != NULL)
> this will never happen when using GetChildGroup(), see above comment.
Done
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 10: Code-Review+2
+2 after rebase
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Kathy Sun (Code Review)" <ge...@cloudera.org>.
Kathy Sun has uploaded a new patch set (#7).
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
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 at
statestored and catalogd
To fix, changed the memz handler method to take an optional metric
group, provided by the caller. memz handler will check the existence of
the metric group.
Used C++11 lambdas rather than boost::bind to help simplify the code.
Testing: Ran locally and looked at impalad/memz, statestored/memz
and catalogd/memz
Add a test file test_web_pages.py to test sending request to /memz on
impalad / statestored / catalogd
Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
---
M be/src/catalog/catalogd-main.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/exec-env.cc
M be/src/scheduling/simple-scheduler.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/util/default-path-handlers.cc
M be/src/util/default-path-handlers.h
M be/src/util/memory-metrics.cc
M be/src/util/metrics-test.cc
M be/src/util/metrics.cc
M be/src/util/metrics.h
A tests/webserver/test_web_pages.py
M www/memz.tmpl
15 files changed, 111 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3998/7
--
To view, visit http://gerrit.cloudera.org:8080/3998
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
......................................................................
Patch Set 9: Code-Review+2
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kathy Sun <ka...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No