You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2016/08/05 22:01:39 UTC
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3851
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
IMPALA-3873: Add QueryStateAccessor
In-flight and completed queries are tracked by different data structures
in ImpalaServer. However, we often want to access both in the same way,
by reading various bits of query metadata that are common to both. This
would involve special-case logic depending on what state the query was
in.
This patch adds ImpalaServer::QueryStateAccessor which wraps either a
QueryExecState or a QueryStateRecord and provides a common struct-like
interface to both. This is mostly useful for the debug webpages that use
these accessors to generate JSON for the query tables etc.
Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
A be/src/service/query-state-accessor.cc
A be/src/service/query-state-accessor.h
6 files changed, 337 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/3851/1
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
PS1, Line 537: class QueryRecordAccessor;
: class QueryExecStateAccessor;
> Are these two forward declarations needed?
Yes - you have to declare a nested class in its parent's declaration, otherwise the definition will not compile.
Line 544: bool GetQueryStateAccessor(const TUniqueId& query_id,
> Why not return the unique_ptr and return an empty one in case the query can
Done
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.cc
File be/src/service/query-state-accessor.cc:
Line 156: bool ImpalaServer::GetQueryStateAccessor(const TUniqueId& query_id,
> This implementation was hard to find. Maybe add a comment in impala-server.
Done
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.h
File be/src/service/query-state-accessor.h:
PS1, Line 32: Clients must lock() and unlock()
> Would it be possible to lock the wrapped objects upon creation and unlock w
This is an interesting idea, but I'd prefer not to. This interface makes it explicit that there is a lock that is taken, and that doing so means that access to the underlying state may be excluding some other reader. Hiding that in the constructor means that clients could unwittingly hold onto the reference for much longer than they would otherwise intend to.
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.h
File be/src/service/query-state-accessor.h:
Line 27: /// proxying common getters to different underlying data structures.
is this supposed to be the end state, or is this class a stop-gap. why would we not consolidate those different data structures? ie, why do we have separate QueryStateRecord and QueryExecState - both of which should be renamed to something containing the string ClientRequest anyway.
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has abandoned this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Abandoned
No time to work on this. Will pick up when I've implemented Marcel's suggestions as a comparison.
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Patch Set 2: Code-Review+1
(6 comments)
Renewed +1, did another pass and noticed a few minor things.
http://gerrit.cloudera.org:8080/#/c/3851/2/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:
Line 757
Was the query_id key deliberately omitted? I'm guessing this was unused?
Line 43: using std::unique_ptr;
Should we add this to common/names.h?
PS2, Line 180: succesfully
successfully
PS2, Line 691: .get()
I don't feel strongly about this, but the .get() isn't necessary for NULL comparisons.
http://gerrit.cloudera.org:8080/#/c/3851/2/be/src/service/query-state-accessor.cc
File be/src/service/query-state-accessor.cc:
Line 161: return unique_ptr<QueryStateAccessor>(new QueryExecStateAccessor(exec_state));
Nit: I find make_unique<QueryStateAccessor>(exec_state) more readable.
Line 168: return unique_ptr<QueryStateAccessor>(
Could use make_unique to get this on one line.
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3851
to look at the new patch set (#2).
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
IMPALA-3873: Add QueryStateAccessor
In-flight and completed queries are tracked by different data structures
in ImpalaServer. However, we often want to access both in the same way,
by reading various bits of query metadata that are common to both. This
would involve special-case logic depending on what state the query was
in.
This patch adds ImpalaServer::QueryStateAccessor which wraps either a
QueryExecState or a QueryStateRecord and provides a common struct-like
interface to both. This is mostly useful for the debug webpages that use
these accessors to generate JSON for the query tables etc.
Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
A be/src/service/query-state-accessor.cc
A be/src/service/query-state-accessor.h
6 files changed, 337 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/3851/2
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
PS1, Line 537: class QueryRecordAccessor;
: class QueryExecStateAccessor;
Are these two forward declarations needed?
Line 544: bool GetQueryStateAccessor(const TUniqueId& query_id,
Why not return the unique_ptr and return an empty one in case the query cannot be found?
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.cc
File be/src/service/query-state-accessor.cc:
Line 156: bool ImpalaServer::GetQueryStateAccessor(const TUniqueId& query_id,
This implementation was hard to find. Maybe add a comment in impala-server.h where to find it?
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.h
File be/src/service/query-state-accessor.h:
PS1, Line 32: Clients must lock() and unlock()
Would it be possible to lock the wrapped objects upon creation and unlock when QueryStateAccessor goes out of scope itself? The places where this change uses it look like the lock_guard always has the same lifetime as the QueryStateAccessor itself.
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3873: Add QueryStateAccessor
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3873: Add QueryStateAccessor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.h
File be/src/service/query-state-accessor.h:
Line 27: /// proxying common getters to different underlying data structures.
> is this supposed to be the end state, or is this class a stop-gap. why woul
the record is the archived version of the query metadata. The exec state is its representation while currently running. In the latter, values may change. In the former, after construction, values are fixed. In the latter, for example, the runtime profile string is generated on demand. In the former, the string is stored in the class (because the information that could generate it has been cleaned up).
This is why there are two separate implementations of roughly the same read-only interface.
Renaming should probably happen when all the query-related classes are renamed.
--
To view, visit http://gerrit.cloudera.org:8080/3851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes