You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2018/09/04 23:54:04 UTC
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11387
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
IMPALA-6568 add missing Query Compilation section to profiles.
The profile command is used to display low-level information about the
most recent query. When a client makes request for the Profile, it
sends a GetRuntimeProfile request for the last queryId to to the server.
The queryId is used to find the ClientRequestState, an object which tracks
information about the execution, including Profile data which is stored in
several RuntimeProfile objects. The reply to the GetRuntimeProfile message
contains the Profile, pretty-printed as a string.
When a query is sent to the Front End to be compiled, the TExecRequest
that is returned from createExecRequest() in the JVM contains a Timeline,
which is a named sequence of events with timing information. This Timeline
is added to the Summary Profile in the ClientRequestState.
In the following cases the Front End was not setting the Timeline in the TExecRequest:
- All DDL operations
- Load data statements
- Set statements
- Explain statements
And this meant that the profile would not contain the "Query Compilation" timeline.
ALTERNATIVES:
To fix this I set the Timeline in TExecRequest before returning it.
> result.setTimeline(timeline.toThrift());
> return result;
I considered writing this as
> return result.setTimeline(timeline.toThrift());
To try to make sure this code gets copied in any future return statements,
but I didn’t think it was worth the cost to readability.
TESTING:
Add a new test to test_observability.py which checks that the "Query
Compilation" timeline appears in the profile for various queries designed
to exercise the new code paths in createExecRequest.
Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/query_test/test_observability.py
2 files changed, 46 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11387/1
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
IMPALA-6568 add missing Query Compilation section to profiles.
The profile command is used to display low-level information about the
most recent query. When a client makes request for the Profile, it
sends a GetRuntimeProfile request for the last queryId to to the server.
The queryId is used to find the ClientRequestState, an object which tracks
information about the execution, including Profile data which is stored in
several RuntimeProfile objects. The reply to the GetRuntimeProfile message
contains the Profile, pretty-printed as a string.
When a query is sent to the Front End to be compiled, the TExecRequest
that is returned from createExecRequest() in the JVM contains a Timeline,
which is a named sequence of events with timing information. This Timeline
is added to the Summary Profile in the ClientRequestState.
In the following cases the Front End was not setting the Timeline in the TExecRequest:
- All DDL operations
- Load data statements
- Set statements
- Explain statements
And this meant that the profile would not contain the "Query Compilation" timeline.
I refactored the main createExecRequest() method to
- try to make the flow clearer,
- allow the timeline to be set in the TExecRequest in only one place.
- to set "Planning finished" in all timelines
TESTING:
Add a new test to test_observability.py which checks that the "Query
Compilation" and "Planning finished" timelines appear in the profile for
various queries designed to exercise the new code paths in createExecRequest.
Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/query_test/test_observability.py
2 files changed, 144 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11387/5
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3134/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Sep 2018 00:25:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11387 )
Change subject: WIP: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
WIP: IMPALA-6568 add missing Query Compilation section to profiles.
WIP: so Todd can look at refactor
TODO: change tests to check that "Planning finished" is present for explain queries
The profile command is used to display low-level information about the
most recent query. When a client makes request for the Profile, it
sends a GetRuntimeProfile request for the last queryId to to the server.
The queryId is used to find the ClientRequestState, an object which tracks
information about the execution, including Profile data which is stored in
several RuntimeProfile objects. The reply to the GetRuntimeProfile message
contains the Profile, pretty-printed as a string.
When a query is sent to the Front End to be compiled, the TExecRequest
that is returned from createExecRequest() in the JVM contains a Timeline,
which is a named sequence of events with timing information. This Timeline
is added to the Summary Profile in the ClientRequestState.
In the following cases the Front End was not setting the Timeline in the TExecRequest:
- All DDL operations
- Load data statements
- Set statements
- Explain statements
And this meant that the profile would not contain the "Query Compilation" timeline.
ALTERNATIVES:
To fix this I set the Timeline in TExecRequest before returning it.
> result.setTimeline(timeline.toThrift());
> return result;
I considered writing this as
> return result.setTimeline(timeline.toThrift());
To try to make sure this code gets copied in any future return statements,
but I didn’t think it was worth the cost to readability.
REFACTOR
I refactored the main createExecRequest() method to try to make the flow clearer.
TESTING:
Add a new test to test_observability.py which checks that the "Query
Compilation" timeline appears in the profile for various queries designed
to exercise the new code paths in createExecRequest.
Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/query_test/test_observability.py
2 files changed, 134 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11387/2
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] WIP: IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: WIP: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1167
PS1, Line 1167: FeFsTable hdfsTable = (FeFsTable) insertStmt.getTargetTable();
> I did not consider this, but I will now.
hmm, but even with the earlier 'planning finished', statements like LOAD DATA don't have the 'Planning finished' in the timeline. I was thinking of taking the entirety of the function body into a new 'doCreateExecStatement', and have the wrapper function create a timeline and be responsible for setting it into the result, sort of similar to the refactor I started here: https://gerrit.cloudera.org/c/11388/2/fe/src/main/java/org/apache/impala/service/Frontend.java#1024
Would that allow you to avoid the repeated calls to result.setTimeline everywhere?
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 19:39:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: WIP: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/591/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 19:53:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1065
PS3, Line 1065: if (!analysisResult.isCreateTableAsSelectStmt()) {
: return result;
: }
> I personally think it's weird, but the impala style is to use single-line i
I tried this but I don't like it. I think its more readable how it is. I don't want to get into a code style war for my first (or second) change... so I will change this if you think it is best, but this doesn't look inconsistent with existing code.
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1134
PS3, Line 1134: private void setMtDopForCatalogOp(AnalysisResult analysisResult,
> can be static? same with several of the extracted functions below
Done
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1150
PS3, Line 1150: createTExecRequest
> maybe rename this to 'createBaseExecRequest' or something? Otherwise it's s
Done
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1164
PS3, Line 1164: AnalysisResult analysisResult,
> it seems this is just used to get the insertStmt, and the insertStmt is alr
Done
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1185
PS3, Line 1185: void addQueryMetadata
> it seems that 'result' is only used as an argument here for storing the fin
Done
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Sep 2018 18:45:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: WIP: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1081
PS2, Line 1081: TQueryExecRequest queryExecRequest = getPlannedExecRequest(queryCtx, analysisResult, timeline, explainString);
line too long (114 > 90)
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1130
PS2, Line 1130: private void setMtDopForCatalogOp(AnalysisResult analysisResult, TQueryOptions queryOptions) {
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1145
PS2, Line 1145: private TExecRequest createTExecRequest(TQueryCtx queryCtx, AnalysisResult analysisResult) {
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1157
PS2, Line 1157: private void addFinalizationParamsForInsert(TQueryCtx queryCtx, AnalysisResult analysisResult,
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1195
PS2, Line 1195: private TQueryExecRequest getPlannedExecRequest(TQueryCtx queryCtx, AnalysisResult analysisResult,
line too long (100 > 90)
http://gerrit.cloudera.org:8080/#/c/11387/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1197
PS2, Line 1197: StringBuilder explainString) throws ImpalaException {
line too long (103 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 20:12:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1167
PS1, Line 1167: timeline.markEvent("Planning finished");
Did you consider refactoring this function to move the guts of it into a new function? It seems a little odd to me that many of the results now have a timeline which doesn't include "Planning finished" - in particular EXPLAIN goes through all of the planning.
http://gerrit.cloudera.org:8080/#/c/11387/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11387/1/tests/query_test/test_observability.py@251
PS1, Line 251: found = False
: for line in runtime_profile.splitlines():
: match = re.search(regex, line)
: if match is not None:
: found = True
maybe just:
assert any(re.search(regex, line) for line in runtime_profiles.splitlines())
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 00:32:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1167
PS1, Line 1167: timeline.markEvent("Planning finished");
> hmm, but even with the earlier 'planning finished', statements like LOAD DA
OK, thanks, I think I see what you mean now.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 22:30:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
IMPALA-6568 add missing Query Compilation section to profiles.
The profile command is used to display low-level information about the
most recent query. When a client makes request for the Profile, it
sends a GetRuntimeProfile request for the last queryId to to the server.
The queryId is used to find the ClientRequestState, an object which tracks
information about the execution, including Profile data which is stored in
several RuntimeProfile objects. The reply to the GetRuntimeProfile message
contains the Profile, pretty-printed as a string.
When a query is sent to the Front End to be compiled, the TExecRequest
that is returned from createExecRequest() in the JVM contains a Timeline,
which is a named sequence of events with timing information. This Timeline
is added to the Summary Profile in the ClientRequestState.
In the following cases the Front End was not setting the Timeline in the TExecRequest:
- All DDL operations
- Load data statements
- Set statements
- Explain statements
And this meant that the profile would not contain the "Query Compilation" timeline.
I refactored the main createExecRequest() method to
- try to make the flow clearer,
- allow the timeline to be set in the TExecRequest in only one place.
- to set "Planning finished" in all timelines
TESTING:
Add a new test to test_observability.py which checks that the "Query
Compilation" and "Planning finished" timelines appear in the profile for
various queries designed to exercise the new code paths in createExecRequest.
Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/query_test/test_observability.py
2 files changed, 146 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11387/4
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1167
PS1, Line 1167: timeline.markEvent("Planning finished");
> Did you consider refactoring this function to move the guts of it into a ne
I did not consider this, but I will now.
I will move
timeline.markEvent("Planning finished");
earlier and see if I can make the rest tidier
http://gerrit.cloudera.org:8080/#/c/11387/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11387/1/tests/query_test/test_observability.py@251
PS1, Line 251: found = False
: for line in runtime_profile.splitlines():
: match = re.search(regex, line)
: if match is not None:
: found = True
> maybe just:
Thanks
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 17:12:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3126/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 06 Sep 2018 21:41:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1065
PS3, Line 1065: if (!analysisResult.isCreateTableAsSelectStmt()) {
: return result;
: }
I personally think it's weird, but the impala style is to use single-line if statements for stuff like this
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1134
PS3, Line 1134: private void setMtDopForCatalogOp(AnalysisResult analysisResult,
can be static? same with several of the extracted functions below
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1150
PS3, Line 1150: createTExecRequest
maybe rename this to 'createBaseExecRequest' or something? Otherwise it's sort of weird that we have 'createExecRequest' and 'createTExecRequest' and it's not quite clear why one does way more than the others.
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1164
PS3, Line 1164: AnalysisResult analysisResult,
it seems this is just used to get the insertStmt, and the insertStmt is already available at the call site. Just take the InsertStmt here directly so it has narrower "scope" (it's more obvious at call sites that this only depends on the insert stmt result)
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1185
PS3, Line 1185: void addQueryMetadata
it seems that 'result' is only used as an argument here for storing the final created TResultSetMetadata. Could you instead return TResultSetMetadata and have the call site look like result.setResult_set_metadata(createQueryResultSetMetadata(analysisResult)) or somesuch? Style-wise I think that's preferred since the function then becomes "pure" (side-effect-free) which is kinda nice.
It also seems this method could be static, which makes it more obvious that there are no side-effects.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Sep 2018 17:33:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/646/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Sep 2018 00:41:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
IMPALA-6568 add missing Query Compilation section to profiles.
The profile command is used to display low-level information about the
most recent query. When a client makes request for the Profile, it
sends a GetRuntimeProfile request for the last queryId to to the server.
The queryId is used to find the ClientRequestState, an object which tracks
information about the execution, including Profile data which is stored in
several RuntimeProfile objects. The reply to the GetRuntimeProfile message
contains the Profile, pretty-printed as a string.
When a query is sent to the Front End to be compiled, the TExecRequest
that is returned from createExecRequest() in the JVM contains a Timeline,
which is a named sequence of events with timing information. This Timeline
is added to the Summary Profile in the ClientRequestState.
In the following cases the Front End was not setting the Timeline in the TExecRequest:
- All DDL operations
- Load data statements
- Set statements
- Explain statements
And this meant that the profile would not contain the "Query Compilation" timeline.
I refactored the main createExecRequest() method to
- try to make the flow clearer,
- allow the timeline to be set in the TExecRequest in only one place.
- to set "Planning finished" in all timelines
TESTING:
Add a new test to test_observability.py which checks that the "Query
Compilation" and "Planning finished" timelines appear in the profile for
various queries designed to exercise the new code paths in createExecRequest.
Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/query_test/test_observability.py
2 files changed, 147 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/11387/3
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@a1069
PS3, Line 1069:
> so was this the main culprit (early return) for omitting the timeline for a
Yes, the early returns were not setting the timeline correctly, which is the source of the bug.
Do you want me to split out the cleanup into a separate jira so it can be better reviewed?
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1034
PS3, Line 1034: t
> nit: uppercase to getTExecRequest
Done
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1035
PS3, Line 1035: StringBuilder explainString)
> indentation here should be 4 spaces from the beginning of the line
I think you mean 6 spaces from the beginning of the line?
So 4 spaces indented from the method's indentation?
I see I was doing this wrong everywhere so I fixed the other cases too.
http://gerrit.cloudera.org:8080/#/c/11387/3/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11387/3/tests/query_test/test_observability.py@221
PS3, Line 221: def test_query_profile_contains_all_events(self):
> use unique_database fixture here so that the various objects here are scope
Thanks for making me learn about pytest :-)
http://gerrit.cloudera.org:8080/#/c/11387/3/tests/query_test/test_observability.py@224
PS3, Line 224: self.hdfs_client.delete_file_dir('tmp/impala6568')
: se
> would it simplify things here if you used an existing file?
I have tidied this up so it is more consistent with other tests
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Sep 2018 23:40:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/577/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Sep 2018 00:26:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Sep 2018 09:50:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 4: Code-Review+1
Looks good to me. There are a few nit-picky formatting things, but rather than listing them all I'll just suggest that you check out clang-format-diff, which can deal with these things for you
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Sep 2018 17:56:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3151/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Sep 2018 06:23:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Sep 2018 03:48:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11387/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11387/1/tests/query_test/test_observability.py@256
PS1, Line 256:
flake8: E221 multiple spaces before operator
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 01:43:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3146/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Sep 2018 00:52:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Sep 2018 06:23:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
IMPALA-6568 add missing Query Compilation section to profiles.
The profile command is used to display low-level information about the
most recent query. When a client makes request for the Profile, it
sends a GetRuntimeProfile request for the last queryId to to the server.
The queryId is used to find the ClientRequestState, an object which tracks
information about the execution, including Profile data which is stored in
several RuntimeProfile objects. The reply to the GetRuntimeProfile message
contains the Profile, pretty-printed as a string.
When a query is sent to the Front End to be compiled, the TExecRequest
that is returned from createExecRequest() in the JVM contains a Timeline,
which is a named sequence of events with timing information. This Timeline
is added to the Summary Profile in the ClientRequestState.
In the following cases the Front End was not setting the Timeline in the TExecRequest:
- All DDL operations
- Load data statements
- Set statements
- Explain statements
And this meant that the profile would not contain the "Query Compilation" timeline.
I refactored the main createExecRequest() method to
- try to make the flow clearer,
- allow the timeline to be set in the TExecRequest in only one place.
- to set "Planning finished" in all timelines
TESTING:
Add a new test to test_observability.py which checks that the "Query
Compilation" and "Planning finished" timelines appear in the profile for
various queries designed to exercise the new code paths in createExecRequest.
Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Reviewed-on: http://gerrit.cloudera.org:8080/11387
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/query_test/test_observability.py
2 files changed, 144 insertions(+), 59 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/620/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Sep 2018 00:19:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@a1069
PS3, Line 1069:
> Yes, the early returns were not setting the timeline correctly, which is th
the refactor is fine.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Sep 2018 06:21:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3112/
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 04:06:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Sep 2018 01:00:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3112/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 00:42:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@a1069
PS3, Line 1069:
so was this the main culprit (early return) for omitting the timeline for all cases (and on L1113)? Main fix is the refactor on L1034. wondering if it makes more sense to keep this change small and deal with additional cleanup separately.
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1034
PS3, Line 1034: t
nit: uppercase to getTExecRequest
http://gerrit.cloudera.org:8080/#/c/11387/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1035
PS3, Line 1035: StringBuilder explainString)
indentation here should be 4 spaces from the beginning of the line
http://gerrit.cloudera.org:8080/#/c/11387/3/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
http://gerrit.cloudera.org:8080/#/c/11387/3/tests/query_test/test_observability.py@221
PS3, Line 221: def test_query_profile_contains_all_events(self):
use unique_database fixture here so that the various objects here are scoped to the test (and cleaned up).
http://gerrit.cloudera.org:8080/#/c/11387/3/tests/query_test/test_observability.py@224
PS3, Line 224: self.hdfs_client.delete_file_dir('tmp/impala6568')
: se
would it simplify things here if you used an existing file?
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Sep 2018 18:18:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Sep 2018 04:16:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11387 )
Change subject: IMPALA-6568 add missing Query Compilation section to profiles.
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/592/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I869eaeb4be4291b6b938f91847f624c76ec90ea5
Gerrit-Change-Number: 11387
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 05 Sep 2018 23:07:10 +0000
Gerrit-HasComments: No