[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/3817/7/be/src/service/query-exec-mgr.cc File be/src/service/query-exec-mgr.cc: Line 85: > The decrementing of ref cnt occurs in ReleaseQueryExecState, which will rem Ah yes, my bad. I've made this a DCHECK -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Sailesh Mukil has uploaded a new patch set (#10). Change subject: IMPALA-4014: Introduce query-wide execution state. .. IMPALA-4014: Introduce query-wide execution state. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now receives incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query, and passes this received fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism and it is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> ClientRequestExecState: This is just a class name change. We still do not include shared state into the QueryExecState because there needs to be changes to DescriptorTbl, etc. before we can incorporate them into the QueryExecState. Will be done as separate patches. P.S: This only shows what the changes would look like and as of yet still does not compile. Excludes: - Some class renames in places not important to the core patch logic. - Most .cc files to make use of changes made. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h R be/src/runtime/fragment-instance-exec-state.cc R be/src/runtime/fragment-instance-exec-state.h R be/src/runtime/fragment-instance-executor.cc R be/src/runtime/fragment-instance-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt R be/src/service/client-request-exec-state.cc R be/src/service/client-request-exec-state.h D be/src/service/fragment-mgr.cc D be/src/service/fragment-mgr.h M be/src/service/impala-internal-service.h A be/src/service/query-exec-mgr.cc A be/src/service/query-exec-mgr.h M common/thrift/ImpalaInternalService.thrift 20 files changed, 735 insertions(+), 360 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/10 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4014: Introduce query-wide execution state.
..
Patch Set 10:
(51 comments)
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-exec-state.h
File be/src/runtime/fragment-instance-exec-state.h:
Line 32: /// Execution state of a single plan fragment.
instance
PS10, Line 33: FragmentInstanceExecState
Let's consider FragmentInstanceState and QueryState (if they're not executing,
what are they doing having state?)
PS10, Line 39: lambda(
:
boost::mem_fn(&FragmentInstanceExecState::ReportStatusCb),
: this, _1, _2, _3)
Try and find a clearer way to write this. If you don't want to write the lambda
in the constructor list, consider a static function that returns a lambda given
'this' as an input:
std::function
MakeCb(FragmentInstanceExecState* fragment_instance) {
return [fragment_instance](const Status& s, RuntimeProfile* profile, bool
done) {
fragment_instance->ReportStatusCb(s, profile, done);
}
}
Please leave a TODO to aggregate all the reports into a single CB managed by
the QES (and file a JIRA as well).
PS10, Line 80: ImpalaBackendClientCache* client_cache_;
Is this necessary given that ExecEnv can give us the same pointer?
PS10, Line 77: QueryExecState* query_exec_state_; // not owned
: TPlanFragmentInstanceCtx fragment_instance_ctx_;
: FragmentInstanceExecutor executor_;
: ImpalaBackendClientCache* client_cache_;
: TExecPlanFragmentParams exec_params_;
Good opportunity to add some comments for these.
PS10, Line 83: plan fragment
fragment instance.
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-executor.cc
File be/src/runtime/fragment-instance-executor.cc:
PS10, Line 80: Close
Since we are aiming for deterministic, manual management of finst lifetimes,
how about moving this out of the destructor and ensuring that the FInst calls
Close() explicitly?
Line 349: // We also want to call sink_->Close() here rather than in
FragmentInstanceExecutor::Close
long line
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-executor.h
File be/src/runtime/fragment-instance-executor.h:
Line 48: /// FragmentInstanceExecutor handles all aspects of the execution of a
single plan fragment,
long line
PS10, Line 155: ExecEnv* exec_env_; // not owned
Can you remove this? ExecEnv::GetInstance() works fine.
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/query-exec-state.cc
File be/src/runtime/query-exec-state.cc:
PS10, Line 60:
remove space
PS10, Line 73:
DCHECK_EQ(fragment_inst_exec_state_map_.find(GetInstanceIdx(fragment_instance_id)),
: fragment_inst_exec_state_map_)
these aren't the same type (does this compile?).
PS10, Line 76: FragmentInstanceExecState* fragment_inst_exec_state =
: new FragmentInstanceExecState(exec_params, this,
ExecEnv::GetInstance());
Presumably this should be managed by obj_pool. If not, the value type of the
map should be unique_ptr<...>.
PS10, Line 80: his RPC returns
This isn't an RPC any more. Update comment.
PS10, Line 113: lexical_cast
PrintId()
PS10, Line 128: lexical_cast
PrintId()
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 38: /// This class is responsible for creating, running and destroying
FragmentInstanceExecStates.
Talk about lifetime here: when is this created, when is it destroyed, what are
the guarantees about the subordinate data structures like finstexecstates. Can
this outlive a query on the coordinator? etc.
PS10, Line 41: // Special constructor for the coordinator.
TODO: remove.
PS10, Line 59: CleanFragmentInstanceStates
Don't talk about implementation details (cleaning the map). When would this be
called?
Line 61: /// Registers a new FragmentInstanceExecState and launches the
thread that calls Prepare() and
long line.
PS10, Line 90: std::unique_ptr obj_pool_
Comment broadly what this is used to manage, since that affects lifecycles.
PS10, Line 90: *
do you mean unique_ptr ?
PS10, Line 90: Owned
redundant wrt unique_ptr
PS10, Line 94: active
expand. What does it mean for a fragment to be active?
PS10, Line 105: shared_ptr
Update comment.
PS10, Line 109: FragmentInstanceExecState
Are the exec states managed by the obj pool? If so, say so otherwise the
lifecycle is not clear.
PS10, Line 113: shared pointer
update comment
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/service/client-request-exec-state.cc
File be/src/service/client-request-exec-state.cc:
Line 17: #include "service/client-request-exec-state.h"
add a blank line before this one
http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/service/client-request-ex
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 10: i'll do the next round once sailesh has addressed henry's comments. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-exec-state.h File be/src/runtime/fragment-instance-exec-state.h: PS10, Line 33: FragmentInstanceExecState > Let's consider FragmentInstanceState and QueryState (if they're not executi i think that's a good idea, long names aren't necessarily more readable or informative. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 10: (52 comments) http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-exec-state.h File be/src/runtime/fragment-instance-exec-state.h: Line 32: /// Execution state of a single plan fragment. > instance Done PS10, Line 33: FragmentInstanceExecState > Let's consider FragmentInstanceState and QueryState (if they're not executi Done PS10, Line 33: FragmentInstanceExecState > i think that's a good idea, long names aren't necessarily more readable or Done PS10, Line 39: lambda( : boost::mem_fn(&FragmentInstanceExecState::ReportStatusCb), : this, _1, _2, _3) > Try and find a clearer way to write this. If you don't want to write the la I'm still not very well versed on the C++ lambda syntax. My next step is to create a compilable version of this patch. I'll make the changes to this in the next patch set so that I get the syntax right and make sure that it compiles. Also, I don't want to hold up reviewers while I figure this out. PS10, Line 80: ImpalaBackendClientCache* client_cache_; > Is this necessary given that ExecEnv can give us the same pointer? No. I've changed it. PS10, Line 77: QueryExecState* query_exec_state_; // not owned : TPlanFragmentInstanceCtx fragment_instance_ctx_; : FragmentInstanceExecutor executor_; : ImpalaBackendClientCache* client_cache_; : TExecPlanFragmentParams exec_params_; > Good opportunity to add some comments for these. Done PS10, Line 83: plan fragment > fragment instance. Done http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-executor.cc File be/src/runtime/fragment-instance-executor.cc: PS10, Line 80: Close > Since we are aiming for deterministic, manual management of finst lifetimes It already does get called explicitly and this call shouldn't be necessary. I changed it to a DCHECK. Line 349: // We also want to call sink_->Close() here rather than in FragmentInstanceExecutor::Close > long line Done http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-executor.h File be/src/runtime/fragment-instance-executor.h: Line 48: /// FragmentInstanceExecutor handles all aspects of the execution of a single plan fragment, > long line Done PS10, Line 155: ExecEnv* exec_env_; // not owned > Can you remove this? ExecEnv::GetInstance() works fine. Done http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/query-exec-state.cc File be/src/runtime/query-exec-state.cc: PS10, Line 60: > remove space Done PS10, Line 73: DCHECK_EQ(fragment_inst_exec_state_map_.find(GetInstanceIdx(fragment_instance_id)), : fragment_inst_exec_state_map_) > these aren't the same type (does this compile?). Oops, sorry. Meant to compare it with map_.end(). Changed it. I haven't gotten this to compile yet, so there may be a few syntax errors that I've missed. PS10, Line 76: FragmentInstanceExecState* fragment_inst_exec_state = : new FragmentInstanceExecState(exec_params, this, ExecEnv::GetInstance()); > Presumably this should be managed by obj_pool. If not, the value type of th Are there any arguments for or against a unique_ptr? It seems to fit the use here perfectly: - We would never want to transfer ownership of the FragmentInstanceState objects. - We want its lifetime to be as long as the QueryState. If there are no objections, I can change it to use a unique_ptr. PS10, Line 80: his RPC returns > This isn't an RPC any more. Update comment. Done PS10, Line 113: lexical_cast > PrintId() Done PS10, Line 128: lexical_cast > PrintId() Done http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/query-exec-state.h File be/src/runtime/query-exec-state.h: Line 38: /// This class is responsible for creating, running and destroying FragmentInstanceExecStates. > Talk about lifetime here: when is this created, when is it destroyed, what Done PS10, Line 41: // Special constructor for the coordinator. > TODO: remove. Done PS10, Line 59: CleanFragmentInstanceStates > Don't talk about implementation details (cleaning the map). When would this Done Line 61: /// Registers a new FragmentInstanceExecState and launches the thread that calls Prepare() and > long line. Done PS10, Line 90: std::unique_ptr obj_pool_ > Comment broadly what this is used to manage, since that affects lifecycles. It doesn't manage anything yet in this patch because we decided to bring in the shared state across fragments (DescriptorTbl) in a future patch. The reason is that the serialization/deserialization of the DescriptorTbl needs to be modified quite a bit to become a general DescTbl across all fragments. So should I remove this since it won't be used for
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Sailesh Mukil has uploaded a new patch set (#11). Change subject: IMPALA-4014: Introduce query-wide execution state. .. IMPALA-4014: Introduce query-wide execution state. This patch is a header preview of a query wide execution state for a backend. Changes in design are as follows: FragmentMgr -> QueryExecMgr The QueryExecMgr now receives incoming fragments, creates a new QueryExecState if it is the first fragment to arrive for that query, and passes this received fragment along to the QueryExecState. This class is responsible for cleaning up the QueryExecState. QueryExecState (new): This is the query wide state for the backend. It is initialized by the first fragment to arrive to the QueryExecMgr. This class is now responsible for creating FragmentExecStates and executing them. Its life is protected by a ref counting mechanism and it is scheduled for destruction once the ref count reaches zero. Once scheduled for destruction, a thread in the QueryExecMgr will destroy the QueryExecState. Every user of the QueryExecState must access it within the scope of ScopedQueryExecStateRef which guarantees ref count incrementing and decrementing at entry/exit of the scope. QueryExecState (old) -> ClientRequestExecState: This is just a class name change. We still do not include shared state into the QueryExecState because there needs to be changes to DescriptorTbl, etc. before we can incorporate them into the QueryExecState. Will be done as separate patches. P.S: This only shows what the changes would look like and as of yet still does not compile. Excludes: - Some class renames in places not important to the core patch logic. - Most .cc files to make use of changes made. Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c --- M be/src/runtime/CMakeLists.txt M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/exec-env.h R be/src/runtime/fragment-instance-exec-state.cc R be/src/runtime/fragment-instance-exec-state.h R be/src/runtime/fragment-instance-executor.cc R be/src/runtime/fragment-instance-executor.h A be/src/runtime/query-exec-state.cc A be/src/runtime/query-exec-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/CMakeLists.txt R be/src/service/client-request-exec-state.cc R be/src/service/client-request-exec-state.h D be/src/service/fragment-mgr.cc D be/src/service/fragment-mgr.h M be/src/service/impala-internal-service.h A be/src/service/query-exec-mgr.cc A be/src/service/query-exec-mgr.h M be/src/util/impalad-metrics.cc M be/src/util/impalad-metrics.h M common/thrift/ImpalaInternalService.thrift 23 files changed, 779 insertions(+), 385 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/11 -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Henry Robinson has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 10: Does this most recent patch compile? If not, how far away are you from getting it working? I think you need to validate that all the abstractions work together as you are expecting, and compilation is a first step in that validation process. Separately, I'm going to respond to some of the open issues from your last review. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4014: Introduce query-wide execution state.
..
Patch Set 11:
(2 comments)
As discussed in person earlier, we decided the best way forward would be to
split this into header changes + implementation (the header changes will allow
Lars to start working on the per-query rpc independently).
We also decided to introduce parallel headers to the existing ones (so we'll
have FragmentInstanceState and FragmentExecState).
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-exec-state.h
File be/src/runtime/fragment-instance-exec-state.h:
Line 18: #ifndef IMPALA_SERVICE_FRAGMENT_EXEC_STATE_H
rename to fragment-instance-state.{cc,h}
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 18: #ifndef IMPALA_RUNTIME_QUERY_EXEC_STATE_H
rename to query-state.{cc,h}
--
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Sailesh Mukil
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 11: > (2 comments) > > As discussed in person earlier, we decided the best way forward > would be to split this into header changes + implementation (the > header changes will allow Lars to start working on the per-query > rpc independently). > > We also decided to introduce parallel headers to the existing ones > (so we'll have FragmentInstanceState and FragmentExecState). I'm currently stuck with trying to solve a crash for IMPALA-3610 and was dealing with an escalation over the weekend. Seeing as how we want to get in 3610 for this release, I will focus on that for today and try to get back to this later tonight. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4014: Introduce query-wide execution state.
..
Patch Set 11:
(43 comments)
this needs a follow-on discussion regarding the existing class hierarchy (and
what it should turn into).
we should disentangle that before adding the per-query rpc functionality.
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-exec-state.h
File be/src/runtime/fragment-instance-exec-state.h:
Line 39: executor_(query_exec_state, exec_env, lambda(
please use a regular c++ lambda
Line 54: Status Prepare();
yet more indirection.
the division of labor between this class and FragmentInstanceExecutor is
unclear (who does the execution). i find the control flow very hard to follow.
here you have to go through the state to get to the executor. and the state in
here is basically all static (exec params) aside from things like status, which
are really execution-related.
Line 77: /// Contains the context for the current fragment instance.
in general: separate w/ blank line if you have member variables with comments
this particular member is redundant, because it's already contained in
exec_params_
Line 84: boost::scoped_ptr exec_thread_;
unused?
if not: who is doing the execution then (who has the execution logic), this
class or InstanceExecutor? this seems convoluted.
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-executor.cc
File be/src/runtime/fragment-instance-executor.cc:
Line 113: cgroup =
ExecEnv::GetInstance()->cgroups_mgr()->UniqueIdToCgroup(PrintId(query_id_,
"_"));
long lines (here and elsewhere)
Line 336: &FragmentInstanceExecutor::ReportProfile, this));
indent 2 spaces for the 2nd level
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-executor.h
File be/src/runtime/fragment-instance-executor.h:
Line 75: /// TODO: Made reports per-query vs per fragment-instance.
make
Line 76: typedef boost::function<
std::
Line 82: FragmentInstanceExecutor(QueryState* query_exec_state, ExecEnv*
exec_env,
might as well also call the param query_state.
drop exec_env, you can call GetInstance()
Line 145: QueryState* query_exec_state() { return query_exec_state_; }
now that we're doing a global rename, let's drop 'state' uniformly
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/query-exec-state.cc
File be/src/runtime/query-exec-state.cc:
Line 64: CreateFragmentInstanceState(exec_params);
does this get called from anywhere else? if not, inline here and get rid of
function. there is too much indirection here, and no clear separation of
responsibilities (in the current code Create does the registration, not
Register).
Line 82: fragment_inst_exec_state_map_.insert(make_pair(GetInstanceIdx(
change line breaks to improve readability (don't break up a function call if
you don't have to)
Line 106: Status QueryState::CancelPlanFragment(const TUniqueId&
fragment_instance_id) {
this and the next function are simply wrapper around FIS member functions, it
looks like you only need GetFIS and the caller can do the rest. this will make
the code easier to follow because it removes a level of indirection
Line 109: if (fragment_inst_exec_state.get() == NULL) {
why not move this into GetInstanceState()?
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
Line 57: // TODO: Remove once we refcator code in coordinator.
spelling
Line 75: void CleanFragmentInstanceStates();
clear?
also, shouldn't this be a general TearDown()?
Line 82: FragmentInstanceState* CreateFragmentInstanceState(
does this need to be public, in addition to the preceding function? who calls
this?
Line 114: /// decrement this to 0 and clean up the query exec state.
move this comment to the actor class (qem).
rephrase (it's the last one to dereference this that will remove the state) if
still useful.
Line 115: int32_t num_current_references_;
not atomic? (or does it even make a difference?)
Line 118: SpinLock fragment_inst_exec_state_map_lock_;
i started using finstance instead of frag_inst or fragment_inst or frag_instance
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
Line 71: QueryState* query_state = NULL);
pass in FIS (which contains a pointer to the query state as well as the exec
params)
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/service/client-request-exec-state.h
File be/src/service/client-request-exec-state.h:
Line 47: /// Execution state of a query and also captures all state required
for servicing
this is limited to client-related state. differentiate more clearly against
QueryState.
Line 57: class ImpalaServer::ClientRequestExecState {
in the interest of removing extraneou
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4014: Introduce query-wide execution state.
..
Patch Set 11:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-exec-state.h
File be/src/runtime/fragment-instance-exec-state.h:
Line 54: Status Prepare();
> yet more indirection.
Could we just make them one class instead?
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/fragment-instance-executor.h
File be/src/runtime/fragment-instance-executor.h:
PS11, Line 49: fragment
fragment instance?
PS11, Line 50: fragment
instance?
PS11, Line 53: fragment
instance?
PS11, Line 85: fragment
instance?
PS11, Line 88: fragment
instance?
Line 145: QueryState* query_exec_state() { return query_exec_state_; }
> now that we're doing a global rename, let's drop 'state' uniformly
drop _exec_?
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:
PS11, Line 46: the the
double word
Line 115: int32_t num_current_references_;
> not atomic? (or does it even make a difference?)
Maybe also comment that this is changed by the QueryExecMgr directly (or add
inc() and dec() method), so it doesn't look like an unused member.
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
PS11, Line 65: /// A collection of items that are part of the global state of a
: /// query and shared across all execution nodes of that query.
How is this different from QueryState now? Is there a way to merge them? If
not, can we elaborate in the comment?
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/service/query-exec-mgr.cc
File be/src/service/query-exec-mgr.cc:
Line 42: Status QueryExecMgr::RegisterFragmentInstanceWithQuery(
> this does more than just registration (it starts a thread that executes the
In case this discussion hasn't happened, could you find a time where I can
participate? Thanks.
PS11, Line 133: query_state_map_
DCHECK that it's in the map?
http://gerrit.cloudera.org:8080/#/c/3817/11/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:
PS11, Line 52: executed
Will it also execute it?
PS11, Line 59: After this call returns, it is legal
Are there cases where it is illegal to call CancelPlanFragment() on the
fragment instance?
PS11, Line 66: The caller
Maybe say "Every caller" so it is more clear that each call to GQS() has to be
matched by a call to RQS()?
PS11, Line 91: should always check
maybe add "check before the first usage", for subsequent calls guarantee 1.
sounded like it will always be != nullptr.
PS11, Line 96: query_exec_mgr_
This member doesn't exists. But DCHECK that ExecEnv::GetQueryExecMgr() exists
sounds like a good idea (also in the d'tor).
PS11, Line 97: ExecEnv
Missing call to GetInstance(), also in d'tor.
PS11, Line 123: ThreadPool destroy_thread_;
The difference between the type and variable name makes this harder to
understand for me.
PS11, Line 137: Calls ReleaseQueryState()
Where does the corresponding increase happen?
--
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: David Knupp
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Sailesh Mukil
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 11: >From the discussion today: Points agreed on: - Collapse Executors and states, i.e. collapse FragmentInstanceExecutor into FragmentInstanceState, so that responsibilities and boundaries of classes are clear. - QueryExecMgr is still responsible for setting up the QueryState and creating the fragment threads for now. Once the per-query RPC is implemented, the model will change such that QueryExecMgr creates a “query" thread which sets up the QueryState and starts the fragment threads. Additionally, I'll put out a separate patch with *only* headers that are parallel with the existing headers, so that Lars can use that to work on the per-query RPC. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4014: Introduce query-wide execution state. .. Patch Set 11: > From the discussion today: > > Points agreed on: > - Collapse Executors and states, i.e. collapse FragmentInstanceExecutor > into FragmentInstanceState, so that responsibilities and boundaries > of classes are clear. > > - QueryExecMgr is still responsible for setting up the QueryState > and creating the fragment threads for now. Once the per-query RPC > is implemented, the model will change such that QueryExecMgr > creates a “query" thread which sets up the QueryState and starts > the fragment threads. Small correction: once we have the per-query rpc we'll still have QEM create the QueryState and register it before starting the query thread (which then starts the fragment instance threads). > > Additionally, I'll put out a separate patch with *only* headers > that are parallel with the existing headers, so that Lars can use > that to work on the per-query RPC. -- To view, visit http://gerrit.cloudera.org:8080/3817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
