[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Tested-by: Impala Public Jenkins --- 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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 13 Sep 2018 09:50:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 13 Sep 2018 06:23:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 13 Sep 2018 06:23:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 13 Sep 2018 06:21:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 12 Sep 2018 04:16:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 12 Sep 2018 00:52:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 12 Sep 2018 00:41:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 11 Sep 2018 17:56:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 08 Sep 2018 03:48:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 08 Sep 2018 00:25:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 08 Sep 2018 00:19:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 07 Sep 2018 23:40:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 07 Sep 2018 18:45:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 07 Sep 2018 18:18:36 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 07 Sep 2018 17:33:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 07 Sep 2018 01:00:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 06 Sep 2018 21:41:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 05 Sep 2018 23:07:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 05 Sep 2018 22:30:47 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 05 Sep 2018 04:06:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 05 Sep 2018 01:43:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 05 Sep 2018 00:42:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 05 Sep 2018 00:32:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6568 add missing Query Compilation section to profiles.
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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 05 Sep 2018 00:26:42 + Gerrit-HasComments: No
[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
