You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2017/01/11 22:28:05 UTC

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline 

Joe McDonnell has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5685

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................

IMPALA-4341: Add metadata load to planner timeline

This moves the timeline from the Analyzer GlobalState to the AnalysisContext
and AnalysisContext.AnalysisResult. When analysis needs to load metadata
about missing tables, it marks an event noting that analysis requires
metadata. Then, when metadata load completes (or times out), it marks an
event noting that metata load completed (or timed out). Keeping the
timeline on the AnalysisContext means that it persists across attempts at
analysis. On successful analysis, AnalysisContext.AnalysisResult has a
reference to the timeline, which is used for the rest of planning.

Here is an example output of the planner timeline after this change:
   Planner Timeline: 5s492ms
       - Analysis Requires Metadata Load: 42.972ms (42.972ms)
       - Metadata Load finished: 5s382ms (5s339ms)
       - Analysis finished: 5s415ms (32.828ms)
       - Equivalence classes computed: 5s434ms (19.356ms)
       - Single node plan created: 5s437ms (2.222ms)
       - Runtime filters computed: 5s438ms (1.666ms)
       - Distributed plan created: 5s440ms (1.495ms)
       - Planning finished: 5s492ms (51.933ms)

When there is no need to load metadata, the timeline looks like:

    Planner Timeline: 3.485ms
       - Analysis finished: 904.409us (904.409us)
       - Equivalence classes computed: 975.323us (70.914us)
       - Single node plan created: 1.306ms (331.209us)
       - Runtime filters computed: 1.335ms (28.773us)
       - Distributed plan created: 1.582ms (247.204us)
       - Planning finished: 3.485ms (1.902ms)

Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 25 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/5685/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5685/1//COMMIT_MSG
Commit Message:

PS1, Line 13: metata
typo


PS1, Line 15: On successful analysis
what happens on unsuccessful analysis? Doesn't it always have that reference?


Line 20:        - Analysis Requires Metadata Load: 42.972ms (42.972ms)
nit: the other entries seem to start words with lowercase.


http://gerrit.cloudera.org:8080/#/c/5685/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

Line 382:       analysisResult_.timeline_ = timeline_;
Move below the next if statement, which checks analyzer is set.


-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has uploaded a new patch set (#2).

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................

IMPALA-4341: Add metadata load to planner timeline

This moves the timeline from the Analyzer GlobalState to the AnalysisContext
and AnalysisContext.AnalysisResult. When analysis needs to load metadata
about missing tables, it marks an event noting the start of metadata load.
Then, when metadata load completes (or times out), it marks an
event noting that metadata load completed (or timed out). Keeping the
timeline on the AnalysisContext means that it persists across attempts at
analysis. AnalysisContext.AnalysisResult has a reference to the timeline,
so that it persists past analyzeStmt and can be used for the rest of
the planning.

Here is an example output of the planner timeline after this change:
    Planner Timeline: 4s371ms
       - Metadata load started: 41.388ms (41.388ms)
       - Metadata load finished: 4s260ms (4s219ms)
       - Analysis finished: 4s296ms (35.693ms)
       - Equivalence classes computed: 4s315ms (19.062ms)
       - Single node plan created: 4s323ms (7.812ms)
       - Runtime filters computed: 4s323ms (777.010us)
       - Distributed plan created: 4s325ms (1.464ms)
       - Planning finished: 4s371ms (46.697ms)

When there is no need to load metadata, the timeline looks like:
    Planner Timeline: 13.695ms
       - Analysis finished: 2.411ms (2.411ms)
       - Equivalence classes computed: 2.653ms (241.733us)
       - Single node plan created: 5.641ms (2.987ms)
       - Runtime filters computed: 5.726ms (85.204us)
       - Distributed plan created: 6.548ms (821.722us)
       - Planning finished: 13.695ms (7.147ms)

Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 23 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/5685/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................


IMPALA-4341: Add metadata load to planner timeline

This moves the timeline from the Analyzer GlobalState to the AnalysisContext
and AnalysisContext.AnalysisResult. When analysis needs to load metadata
about missing tables, it marks an event noting the start of metadata load.
Then, when metadata load completes (or times out), it marks an
event noting that metadata load completed (or timed out). Keeping the
timeline on the AnalysisContext means that it persists across attempts at
analysis. AnalysisContext.AnalysisResult has a reference to the timeline,
so that it persists past analyzeStmt and can be used for the rest of
the planning.

Here is an example output of the planner timeline after this change:
    Planner Timeline: 4s371ms
       - Metadata load started: 41.388ms (41.388ms)
       - Metadata load finished: 4s260ms (4s219ms)
       - Analysis finished: 4s296ms (35.693ms)
       - Equivalence classes computed: 4s315ms (19.062ms)
       - Single node plan created: 4s323ms (7.812ms)
       - Runtime filters computed: 4s323ms (777.010us)
       - Distributed plan created: 4s325ms (1.464ms)
       - Planning finished: 4s371ms (46.697ms)

When there is no need to load metadata, the timeline looks like:
    Planner Timeline: 13.695ms
       - Analysis finished: 2.411ms (2.411ms)
       - Equivalence classes computed: 2.653ms (241.733us)
       - Single node plan created: 5.641ms (2.987ms)
       - Runtime filters computed: 5.726ms (85.204us)
       - Distributed plan created: 6.548ms (821.722us)
       - Planning finished: 13.695ms (7.147ms)

Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Reviewed-on: http://gerrit.cloudera.org:8080/5685
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 23 insertions(+), 15 deletions(-)

Approvals:
  Marcel Kornacker: Looks good to me, approved
  Impala Public Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................


Patch Set 2: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5685/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

Line 64:   // Timeline of important events in the planning process, used for debugging
debugging and profiling


http://gerrit.cloudera.org:8080/#/c/5685/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

Line 908:           analysisCtx.getTimeline().markEvent("Analysis Requires Metadata Load");
let's phrase that as an activity, for instance "Metadata load started"


Line 916:             analysisCtx.getTimeline().markEvent("Metadata Load timeout");
"Metadata load" (to go along with existing timeline output)


Line 917:           }
formatting


-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4341: Add metadata load to planner timeline

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4341: Add metadata load to planner timeline
......................................................................


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/171/

-- 
To view, visit http://gerrit.cloudera.org:8080/5685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f01a35e5f9f5007a0298acfc8e16da00ef99c6c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No