[Impala-ASF-CR] IMPALA-7335: Fix a race in HdfsScanNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11337 ) Change subject: IMPALA-7335: Fix a race in HdfsScanNode .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.h@181 PS6, Line 181: Calling it repeatedly ignores subsequent : /// calls. The first non-ok status is set and all subsequent errors are ignored. those two sentences seem to contradict each other. If calling it repeatedly ignores subsequent calls then subsequent errors would also be ignored. http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc@512 PS6, Line 512: marking a range as complete. I don't think it's obvious where that happens. Is that in the Close() call at line 519? If so, maybe make that more explicit to clarify the dependency. http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc@538 PS6, Line 538: // The scan node's execution completed, preserve the status_ currently set. does status_ still get set outside of SetDoneInternal()? If so, why? -- To view, visit http://gerrit.cloudera.org:8080/11337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id470818846a5c69ad28a6ff695069702982aa793 Gerrit-Change-Number: 11337 Gerrit-PatchSet: 6 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 17 Sep 2018 19:01:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6202. mod and % are not equivalent.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11443 ) Change subject: IMPALA-6202. mod and % are not equivalent. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11443/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test File testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test: http://gerrit.cloudera.org:8080/#/c/11443/1/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test@516 PS1, Line 516: # IMPALA-6202 - mod and % are not equivalent. rather than talk about how the old code used to work, it'd be better to word this in the way the code should work: "MOD and % should be equivalent". The current wording could be interpreted to mean that they are currently not equivalent. -- To view, visit http://gerrit.cloudera.org:8080/11443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0067da04083859ffbf662a29007154461bea2ba Gerrit-Change-Number: 11443 Gerrit-PatchSet: 1 Gerrit-Owner: Yongjun Zhang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Sep 2018 17:41:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7348: fix race in KuduUtil.getKuduClient
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11423 ) Change subject: IMPALA-7348: fix race in KuduUtil.getKuduClient .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11423 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec50e9b592593cea5418e7a074bdda065184086f Gerrit-Change-Number: 11423 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 11 Sep 2018 22:09:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 21: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 21 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Tue, 11 Sep 2018 20:04:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: undefined behavior: codegen signed overflow
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11406 ) Change subject: IMPALA-5031: undefined behavior: codegen signed overflow .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc File be/src/exprs/operators-ir.cc: http://gerrit.cloudera.org:8080/#/c/11406/1/be/src/exprs/operators-ir.cc@56 PS1, Line 56: Modern compilers can "see through" the call to memcpy and generate code with no copies. did you verify that it's happening? what about in debug builds? wouldn't a static cast work just the same? i.e. unsigned x = (unsigned)y; though I don't feel strongly if you're sure the memcpy really goes away in all cases. -- To view, visit http://gerrit.cloudera.org:8080/11406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I79ec3a5ed974709e5e47be6b074d39ee89461f7f Gerrit-Change-Number: 11406 Gerrit-PatchSet: 1 Gerrit-Owner: Jim Apple Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 11 Sep 2018 17:46:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 20: (3 comments) Thanks, I find following the sequence much more intuitive with the adjusted terminology. http://gerrit.cloudera.org:8080/#/c/10744/20/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10744/20/be/src/service/impala-server.h@115 PS20, Line 115: /// learned that the executor was quiescing). it might be worthwhile to point out explicitly that admission control queuing plays a big factor in this latency http://gerrit.cloudera.org:8080/#/c/10744/20/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java File fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java: http://gerrit.cloudera.org:8080/#/c/10744/20/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@36 PS20, Line 36: * Represents an administrative function call, e.g. ": shutdown()". it might be helpful to show the optional parameters too in the comment. http://gerrit.cloudera.org:8080/#/c/10744/20/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@55 PS20, Line 55: private long deadlineSecs_; why is it that the shutdown deadline can be overridden by the command, but not the startup grace period? -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 20 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Tue, 11 Sep 2018 17:22:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command
..
Patch Set 16:
(15 comments)
This generally looks good to me. Mostly minor comments or thoughts on
terminology.
http://gerrit.cloudera.org:8080/#/c/10744/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10744/16//COMMIT_MSG@40
PS16, Line 40: 4. the quiesce period (which is ideally set to the AC queueing
: delay plus some additional leeway) expires
: 5. once the daemon is drained (i.e. no fragments, no registered
: queries), it shuts itself down.
5 seems like also a form of quiescing (or continuation of the quiescing
process), so the name quiesce deadline seemed confusing to me (i.e. i was
expecting that to be the deadline described in 6 - i.e. quiescing of this
daemon hasn't completed).
Maybe call the thing in #4 the admission grace period or query startup grace
period or something? if others find the current terminology self explanatory,
okay to ignore.
http://gerrit.cloudera.org:8080/#/c/10744/16//COMMIT_MSG@45
PS16, Line 45: longer timeout
when does the clock start on this? after 4 or at the time of the request?
http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/scheduling/scheduler.cc@191
PS16, Line 191: is_quiescing
presumably this remains set after the "quiescing deadline" has passed, which is
more reason the name of that deadline can be confusing.
http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.h@1196
PS16, Line 1196:
... registered queries and ... ?
http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.cc@216
PS16, Line 216: started queries to complete executing before
I found this a bit confusing: isn't it more the period for queries to finish
starting, i.e. period before we'll use metrics to determine whether the node
has actually quiesced?
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@513
PS16, Line 513: enum TAdminRequestType {
would be good to comment the struct
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@518
PS16, Line 518: the statement
what's "the statement"?
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@519
PS16, Line 519: If the port was specified, it is set in 'backend'. If
: // it was not specified, it is 0 and the port configured for
this Impala daemon is
: // assumed.
do we need these multiple cases? is that to make it easier to test in the
mini-cluster, or is there a different reason for that?
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@528
PS16, Line 528: struct TAdminRequest {
comment the struct
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@531
PS16, Line 531: 2: optional TShutdownParams shutdown_params
presumably only one of these lists would be set, corresponding to the type tag?
i.e. this is a union?
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift@857
PS16, Line 857: // Deadline for the shutdown.
what happens at the deadline? is that documented somewhere else?
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift@872
PS16, Line 872: registered
what does that mean? Registered in the "impala-server" sense for coordinators,
or something else?
http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift@881
PS16, Line 881: 2: required TShutdownStatus shutdown_status
given the fields in status, does the caller make multiple calls to
RemoteShutdown() to get updated status, or something?
http://gerrit.cloudera.org:8080/#/c/10744/16/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/10744/16/fe/src/main/cup/sql-parser.cup@2509
PS16, Line 2509: COLON
do other systems use this syntax? how was it chosen (other than to avoid
keywords)?
http://gerrit.cloudera.org:8080/#/c/10744/16/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
File fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java:
http://gerrit.cloudera.org:8080/#/c/10744/16/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.
[Impala-ASF-CR] IMPALA-7446: enable missing buffer pool GC
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11350 ) Change subject: IMPALA-7446: enable missing buffer pool GC .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/11350/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11350/3//COMMIT_MSG@10 PS3, Line 10: can spill why is that? isn't the memory available for BP buffers regardless of whether it's in the buffer pool free list or the system? http://gerrit.cloudera.org:8080/#/c/11350/3//COMMIT_MSG@10 PS3, Line 10: OOM how did the OOM (non spillable allocation failures) go unnoticed for so long? what tests are missing (besides unit test)? -- To view, visit http://gerrit.cloudera.org:8080/11350 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32400bda18a36f60cbe315fae715748d33677c10 Gerrit-Change-Number: 11350 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 30 Aug 2018 19:05:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7464: fix race when ExecFInstance() RPC times out
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11339 ) Change subject: IMPALA-7464: fix race when ExecFInstance() RPC times out .. Patch Set 3: Code-Review+2 (4 comments) Carry +2 http://gerrit.cloudera.org:8080/#/c/11339/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11339/2//COMMIT_MSG@24 PS2, Line 24: aren't particularly good at verifying it > We actually pass a RuntimeState* into PlanRootSink::GetNext() from the coor yeah, that's true, and agree that's weird. http://gerrit.cloudera.org:8080/#/c/11339/2/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/11339/2/be/src/runtime/query-state.h@133 PS2, Line 133: back > backend Done http://gerrit.cloudera.org:8080/#/c/11339/2/be/src/runtime/query-state.h@152 PS2, Line 152: back > backend? Done http://gerrit.cloudera.org:8080/#/c/11339/2/be/src/runtime/query-state.h@408 PS2, Line 408: M > extra space Done -- To view, visit http://gerrit.cloudera.org:8080/11339 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If60d983e0e68b00e6557185db1f86757ab8b3f2d Gerrit-Change-Number: 11339 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 29 Aug 2018 01:07:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7464: fix race when ExecFInstance() RPC times out
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11339 to look at the new patch set (#3). Change subject: IMPALA-7464: fix race when ExecFInstance() RPC times out .. IMPALA-7464: fix race when ExecFInstance() RPC times out The "exec resources" reference count on the QueryState expects that it will transition from 0 -> non-zero -> 0 at most once. The reference count is taken on the coordinator side (sender of this RPC) and also the backend (receiver of this RPC). Usually, the lifetimes of those references overlap (the coordinator won't give up the reference until the backend execution is complete or failed), and so the assumption is not violated. However, when the RPC times out, the receiver may run after the sender has given up its reference (since the sender doesn't know the receiver is actually still executing). As it turns out, the coordinator doesn't really need to take a reference given the current code (verified via code inspection), as these resources are backend-only). So, stop taking the reference on the coordinator side, and add some DCHECKs to document that (the dchecks aren't particularly good at verifying it, however, since the lifetimes generally will overlap). Note that this patch can't be easily backported to older versions without careful inspection since older versions of the code may have relied on the reference count protecting things used by the coordinator. Testing: - New test_rpc_timeout case that reproduced the problem 100% - exhaustive build Change-Id: If60d983e0e68b00e6557185db1f86757ab8b3f2d --- M be/src/benchmarks/process-wide-locks-benchmark.cc M be/src/runtime/coordinator.cc M be/src/runtime/query-exec-mgr.cc M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/test-env.cc M tests/custom_cluster/test_rpc_timeout.py 8 files changed, 76 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/11339/3 -- To view, visit http://gerrit.cloudera.org:8080/11339 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If60d983e0e68b00e6557185db1f86757ab8b3f2d Gerrit-Change-Number: 11339 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7335: Fix a race in HdfsScanNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11337 ) Change subject: IMPALA-7335: Fix a race in HdfsScanNode .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11337/3/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/3/be/src/exec/hdfs-scan-node.cc@516 PS3, Line 516: before marking a range as complete. thanks, that's helpful. but why do we mark a range as complete if we get an error here, whereas if StartNextScanRange() errors, we presumably don't do that. In other words, maybe another solution to the race is to no "complete" the range if there was an error. Or, it seems that both error paths really could be common (i.e. why not mark the range complete in the case of StartNextScanRange() error). Hmm, maybe there's something subtle for abort_on_error=false, though, so maybe don't muck with that after all. Anyway, I'm fine with not trying to do too much cleanup on this right now (besides Tim's refactoring suggestion), if it will eventually be generally cleaned up. -- To view, visit http://gerrit.cloudera.org:8080/11337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id470818846a5c69ad28a6ff695069702982aa793 Gerrit-Change-Number: 11337 Gerrit-PatchSet: 3 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 29 Aug 2018 01:02:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7335: Fix a race in HdfsScanNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11337 ) Change subject: IMPALA-7335: Fix a race in HdfsScanNode .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11337/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/2/be/src/exec/hdfs-scan-node.cc@515 PS2, Line 515: Ensure that : // status_ is updated before the done_ flag. that was already true in the old code, no? Is the requirement also that this happens before scanner->Close()? It's unfortunate that this code is still duplicated here and in the caller and the errors are handled asymmetrical. An alternative would be to move lines 526-530 into the caller, but I guess that might be just as messy. -- To view, visit http://gerrit.cloudera.org:8080/11337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id470818846a5c69ad28a6ff695069702982aa793 Gerrit-Change-Number: 11337 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 28 Aug 2018 18:27:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7464: fix race when ExecFInstance() RPC times out
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11339 ) Change subject: IMPALA-7464: fix race when ExecFInstance() RPC times out .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11339/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11339/2//COMMIT_MSG@21 PS2, Line 21: (verified via code inspection) I'd appreciate the code reviewer to verify this as well. http://gerrit.cloudera.org:8080/#/c/11339/2//COMMIT_MSG@24 PS2, Line 24: aren't particularly good at verifying it I thought about requiring a RuntimeState to be passed when accessing these resources (which would prove that the accesses are coming from the backend side) but that seemed cumbersome. -- To view, visit http://gerrit.cloudera.org:8080/11339 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If60d983e0e68b00e6557185db1f86757ab8b3f2d Gerrit-Change-Number: 11339 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 28 Aug 2018 17:11:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7464: fix race when ExecFInstance() RPC times out
Dan Hecht has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11339 ) Change subject: IMPALA-7464: fix race when ExecFInstance() RPC times out .. IMPALA-7464: fix race when ExecFInstance() RPC times out The "exec resources" reference count on the QueryState expects that it will transition from 0 -> non-zero -> 0 at most once. The reference count is taken on the coordinator side (sender of this RPC) and also the backend (receiver of this RPC). Usually, the lifetimes of those references overlap (the coordinator won't give up the reference until the backend execution is complete or failed), and so the assumption is not violated. However, when the RPC times out, the receiver may run after the sender has given up its reference (since the sender doesn't know the receiver is actually still executing). As it turns out, the coordinator doesn't really need to take a reference given the current code (verified via code inspection), as these resources are backend-only). So, stop taking the reference on the coordinator side, and add some DCHECKs to document that (the dchecks aren't particularly good at verifying it, however, since the lifetimes generally will overlap). Note that this patch can't be easily backported to older versions without careful inspection since older versions of the code may have relied on the reference count protecting things used by the coordinator. Testing: - New test_rpc_timeout case that reproduced the problem 100% - exhaustive build Change-Id: If60d983e0e68b00e6557185db1f86757ab8b3f2d --- M be/src/benchmarks/process-wide-locks-benchmark.cc M be/src/runtime/coordinator.cc M be/src/runtime/query-exec-mgr.cc M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/test-env.cc M tests/custom_cluster/test_rpc_timeout.py 8 files changed, 76 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/11339/2 -- To view, visit http://gerrit.cloudera.org:8080/11339 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If60d983e0e68b00e6557185db1f86757ab8b3f2d Gerrit-Change-Number: 11339 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] IMPALA-7349: Add Admission control support for automatically setting mem limit
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11157 ) Change subject: IMPALA-7349: Add Admission control support for automatically setting mem_limit .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@10 PS7, Line 10: With this patch the mem_limit of a query is automatically set based on : the mem_limit set in the query options and the mem_estimate calculated : by the planner. > the min/max limits are not accessible through query options, they are a par Ah, okay. In that case, maybe clamp-mem-limit-query-option isn't such a bad name, but up to you. -- To view, visit http://gerrit.cloudera.org:8080/11157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifec00141651982f5975803c2165b7d7a10ebeaa6 Gerrit-Change-Number: 11157 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 27 Aug 2018 23:01:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7335: Fix a race in HdfsScanNode
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/11337 )
Change subject: IMPALA-7335: Fix a race in HdfsScanNode
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc@398
PS1, Line 398: // If there was already an error, the main thread will do
the cleanup
: if (!status_.ok()) break;
does that still work as expected now that ProcessSplit already set status_?
http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc@401
PS1, Line 401: if (status.IsCancelled() && done_) {
: // Scan node initiated scanner thread cancellation. No
need to do anything.
: break;
: }
: // Set status_ before calling SetDone() (which shuts down
the RowBatchQueue),
: // to ensure that GetNextInternal() notices the error
status.
: status_ = status;
: SetDoneInternal();
: break;
: }
can this be removed now that it's handled in ProcessSplit()? Hmm, I guess we
still need it for the case that StartNextScanRange() returned non-OK, but I'd
make this more explicit so it's clearer how this all works.
http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc@514
PS1, Line 514: // HdfsScanNodeBase::status_ variable and notify other
scanner threads.
It seems like the whole scanner thread protocol could really use a cleanup. But
if you want to go with this kind of fix in the meantime, it'd be good to
explicitly state in the comment what the dependency is here -- that status_
needs to be set before whatever it is.
--
To view, visit http://gerrit.cloudera.org:8080/11337
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id470818846a5c69ad28a6ff695069702982aa793
Gerrit-Change-Number: 11337
Gerrit-PatchSet: 1
Gerrit-Owner: Pooja Nilangekar
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 27 Aug 2018 21:42:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7349: Add Admission control support for automatically setting mem limit
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11157 ) Change subject: IMPALA-7349: Add Admission control support for automatically setting mem_limit .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@10 PS7, Line 10: With this patch the mem_limit of a query is automatically set based on : the mem_limit set in the query options and the mem_estimate calculated : by the planner. > the reasoning behind clamping mem_limit is to prevent a case where a user c That makes sense, but of course the user can also override these min/max query options as well. But I suppose that could change in the future. I think it'd be helpful to update the commit message to clarify some of how this works (the pseudo code you wrote above). http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@18 PS7, Line 18: strict-min-max-query-mem-limit > this is a bit unclear, but not sure I have a better suggestion. In any case >From your description of what this does, I was going to suggest something like >"clamp_mem_limit_option", which is maybe a bit more specific and descriptive >of what it actually does (based on this discussion) compared to "strict min >max query mem limit". But I don't really like that suggestion more than what >you have. But maybe someone will think of an improvement from that. -- To view, visit http://gerrit.cloudera.org:8080/11157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifec00141651982f5975803c2165b7d7a10ebeaa6 Gerrit-Change-Number: 11157 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 27 Aug 2018 21:01:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7349: Add Admission control support for automatically setting mem limit
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11157 ) Change subject: IMPALA-7349: Add Admission control support for automatically setting mem_limit .. Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@10 PS7, Line 10: With this patch the mem_limit of a query is automatically set based on : the mem_limit set in the query options and the mem_estimate calculated : by the planner. > For the new behavior (Assuming either min or max memlimit is set in the poo In the "else" clause, I understand why we have the min/max clamping values (because we're using heuristics to cook up a mem_limit). But in the "then" clause, why does it make sense to override the mem_limit if it was explicitly set by the user? Also, in terms of terminology in this part, my suggestion would be to come up with a different term rather than mem_limit for the thing before it gets clamped, and keep using mem_limit to mean only the final value used to limit memory consumption. But it's difficult to do that given the "then" clause and the pre-existing mem_limit query option name, which is another reason for my question above. http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@15 PS7, Line 15: in-query-mem-limit-bytes, max-query-mem-limit-bytes these names seem okay. http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@18 PS7, Line 18: strict-min-max-query-mem-limit this is a bit unclear, but not sure I have a better suggestion. In any case, let's finish the discussion above first anyway. -- To view, visit http://gerrit.cloudera.org:8080/11157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifec00141651982f5975803c2165b7d7a10ebeaa6 Gerrit-Change-Number: 11157 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 22 Aug 2018 17:09:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] cleanup: remove RuntimeState::exec env
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11269 ) Change subject: cleanup: remove RuntimeState::exec_env_ .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11269 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a1b1bdd41e3d10982b3a4bdb0217e716b4df67f Gerrit-Change-Number: 11269 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 21 Aug 2018 23:36:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7349: Add Admission control support for automatically setting mem limit
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11157 ) Change subject: IMPALA-7349: Add Admission control support for automatically setting mem_limit .. Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@10 PS7, Line 10: With this patch the mem_limit of a query is automatically set based on : the mem_limit set in the query options and the mem_estimate calculated : by the planner. could you go into more detail what this means? how is the "output" mem_limit computed from the query option "mem_limit"? And does that mean the query option mem_limit is no longer a limit on memory consumption? http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@16 PS7, Line 16: If these are set to zero is it if any or all are set to zero? http://gerrit.cloudera.org:8080/#/c/11157/7//COMMIT_MSG@18 PS7, Line 18: if false, the mem_limit defined in : the query options is used directly and the max/min limits are not : enforced on it. does that mean this option effectively disables this new behavior entirely? or does it disable only parts of it? -- To view, visit http://gerrit.cloudera.org:8080/11157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifec00141651982f5975803c2165b7d7a10ebeaa6 Gerrit-Change-Number: 11157 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 21 Aug 2018 23:29:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7422: Fix a race in QueryState::StartFInstances()
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11270 ) Change subject: IMPALA-7422: Fix a race in QueryState::StartFInstances() .. Patch Set 2: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/11270/2/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/11270/2/be/src/runtime/query-state.h@345 PS2, Line 345: accessor reader? Since writes by the startup thread are allowed, and in fact required to be, earlier, right? http://gerrit.cloudera.org:8080/#/c/11270/2/be/src/runtime/query-state.h@351 PS2, Line 351: accessor readers -- To view, visit http://gerrit.cloudera.org:8080/11270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I35f2a5b0ea5143703850ffc229cec0e4294e6a3e Gerrit-Change-Number: 11270 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 20 Aug 2018 20:46:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7096: restore scanner thread memory heuristics
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11103 ) Change subject: IMPALA-7096: restore scanner thread memory heuristics .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/11103/8/be/src/runtime/scanner-mem-limiter.cc File be/src/runtime/scanner-mem-limiter.cc: http://gerrit.cloudera.org:8080/#/c/11103/8/be/src/runtime/scanner-mem-limiter.cc@32 PS8, Line 32: ScanNode* const node; it might be clearer to remove that now (so there's no question as to whether this equals the map key or not). -- To view, visit http://gerrit.cloudera.org:8080/11103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9907fa8c4d2b0b85f67f4f160899c1c258ad82b Gerrit-Change-Number: 11103 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Aug 2018 21:29:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7096: restore scanner thread memory heuristics
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11103 ) Change subject: IMPALA-7096: restore scanner thread memory heuristics .. Patch Set 7: Code-Review+2 (6 comments) http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h File be/src/runtime/scanner-mem-limiter.h: http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@32 PS7, Line 32: limiting the number of scanner threads is it to limit the number of scanner threads, or the aggregate memory consumption by scanner threads? http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@42 PS7, Line 42: as long until garbled http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@43 PS7, Line 43: tears down all control structures. is that requirement because the instance of this class happens to also be a QueryState control structure? If so, maybe clearer to just say node must outlive the lifetime of this object? (i.e. to indicate that this object is gonna keep a reference to it). Since, it's not really dictated by this abstraction who owns the instance of '*this'. http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@71 PS7, Line 71: std::vector> registered_scans_; do we need that? why not just iterate over the map? do we want ordering but don't want to use map<>? http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc File be/src/runtime/scanner-mem-limiter.cc: http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc@75 PS7, Line 75: This is carried over from old versions of the : // code. what code? before this change or before the change that removed the original heuristic? only because this is so arbitrary, it might help be be more specific in this reference. http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc@81 PS7, Line 81: // Add the expected increase in consumption for existing threads. : addtl_consumption += static_cast(consumption * 0.5); I don't really understand that. Why would adding this thread increase consumption of the other threads? oh, maybe this is just saying that we're guessing in the future the threads may happen to grow by 50% over their current usage? -- To view, visit http://gerrit.cloudera.org:8080/11103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9907fa8c4d2b0b85f67f4f160899c1c258ad82b Gerrit-Change-Number: 11103 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 15 Aug 2018 18:03:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7396: unblock testing with --thread creation fault injection
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11123 ) Change subject: IMPALA-7396: unblock testing with --thread_creation_fault_injection .. Patch Set 5: > There were at least two layers: > 1. The Coordinator was not in a terminal state, so it DCHECKed in > the destructor. > 2. I tried a few things to avoid that, including disabling the > DCHECK and cancelling the query, but I still ended up hitting > various lifecycle issues, e.g. backend_exec_complete_barrier_ was > NULL. Is there a JIRA for fixing those (don't see it linked to IMPALA-7396)? -- To view, visit http://gerrit.cloudera.org:8080/11123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc9da645c449a266e3a54d0e35b18a2823019cb7 Gerrit-Change-Number: 11123 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 13 Aug 2018 23:43:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove some undefined behavior of NULL pointers
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/11195 )
Change subject: IMPALA-5031: Remove some undefined behavior of NULL pointers
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11195/2/be/src/util/ubsan.h
File be/src/util/ubsan.h:
http://gerrit.cloudera.org:8080/#/c/11195/2/be/src/util/ubsan.h@21
PS2, Line 21: mimicking parts of the standard prone
I'm not sure what that's trying to say. How is this mimicking? Isn't it
guarding against?
http://gerrit.cloudera.org:8080/#/c/11195/2/be/src/util/ubsan.h@35
PS2, Line 35: static void* MemCpy(void* dest, const void* src, size_t n) {
is the point of these wrappers to allow nullptr when that's normally undefined?
probably clarifying the top level comment would explain that.
--
To view, visit http://gerrit.cloudera.org:8080/11195
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c8a6624918389396789a83b32dbf068b9327f76
Gerrit-Change-Number: 11195
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 13 Aug 2018 20:36:03 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7163: Implement a state machine for the QueryState class
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10813 )
Change subject: IMPALA-7163: Implement a state machine for the QueryState class
..
Patch Set 3:
(21 comments)
I wasn't able to get through another entire iteration but here's some comments.
Michael will have to take over this review.
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@65
PS3, Line 65: query state
I think we should avoid using the term "query state" given that this class is
called QueryState but we mean something else :) How about just saying "a state"
and then refer to it as BackendExecState or backend exec state in future
references?
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@65
PS3, Line 65: BackedExecState
Backend
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@68
PS3, Line 68: to the corresponding error
are there multiple error states? corresponding implies that. Or are you
including CANCELLED here?
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@72
PS3, Line 72: This is to avoid bugs in
: /// the query lifecycle that were fixed as part of IMPALA-2550.
Let's not state in terms of past code. How about:
This is to simplify the query lifecycle so that Prepare() is always completed
before handling a cancel RPC.
(I assume that's what we're talking about?)
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@149
PS3, Line 149: Blocks until all fragment instances have finished their Prepare
phase.
put another way, I guess this blocks until threads have exited INITIALIZED?
Does it make sense to keep StartFInstances() and MonitorFInstances() separately?
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@155
PS3, Line 155: /// Not idempotent, not thread-safe. Must only be called by a
single thread.
it it required that this is called after StartFInstances()?
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@210
PS3, Line 210: typedef struct FInstanceStatus FInstanceStatus;
Is that needed in C++? You'd do it for C but I think C++ effectively includes
the typedef automatically.
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@213
PS3, Line 213: Function
nit: here and below you could delete the word "Function" since that's obvious
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@266
PS3, Line 266: Cancel
Isn't Cancel() called in case of error as well? If so, does this mean we go
through CANCELLED and ERROR in that case?
If not, should this say CancelFInstances() RPC was handled while executing.
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@271
PS3, Line 271: query_exec_state_
how about backend_exec_state_ for consistency?
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@284
PS3, Line 284: &
here and elsewhere: given BackendExecState is a scalar (enum) value, should
just pass by value.
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@286
PS3, Line 286: form
from
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@305
PS3, Line 305: status
overall status
http://gerrit.cloudera.org:8080/#/c/10813/3/be/src/runtime/query-state.h@306
PS3, Line 306: /// Status::OK if all the fragment instances managed by this
QS are also Status::OK
otherwise it's the status of the first non-OK FIS?
Will this thing become a FInstancesStatus (so you can report back which FIS
failed)?
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc@437
PS2, Line 437:
: }
: return prepare_status;
: }
:
: QueryState::FInstanceStatus QueryState::WaitForFinishInternal() {
: return instances_finished_barrier_->Wait();
: }
:
: void QueryState::StartFInstances() {
: VLOG_QUERY << "StartFInstances(): query_id=" <<
PrintId(query_id())
: << " #instances=" <<
rpc_params_.fragment_instance_ctxs.size();
: DCHECK_GT(refcnt_.Load(), 0);
: DCHECK_GT(exec_resource_refcnt_.Load(), 0) << "Should have
been taken in Init()";
:
> The Prepare() case is special as we historically don't let any action happe
My point was shouldn't the counting barrier take care of that for us? I.e. call
Notify() rather than NotifyRemaining(). This loop is effectively implementing
the barrier in yet another way. I get that you want to have the status from the
first failed but wait for the last to finish prepare, but it
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 ) Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.cc File be/src/exec/grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.cc@91 PS4, Line 91: is_streaming_preagg_(tnode.agg_node.use_streaming_preaggregation), > I'm not sure having a TAggregationNode and a TStreamingAggregationNode is t Okay, leaving it works for me. -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 29 Jun 2018 00:22:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 ) Change subject: IMPALA-7215: Implement a templatized CountingBarrier .. Patch Set 4: Code-Review+2 (4 comments) Just a few more comment cleanups for the public interface. http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h File be/src/util/counting-barrier.h: http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@36 PS4, Line 36: returned value Not clear which "returned value" this is talking about - sounds like Notify()'s returned value. The comment for NotifyRemaining is clearer http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@43 PS4, Line 43: unblocks Wait() with : /// the returned value as 'promise_value'. that's clearer. you could say it this way for Notify() comment. http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@56 PS4, Line 56: 'promise_' public comments shouldn't talk about private fields (the client of this class shouldn't care that there is a promise in the implementation). Something like: Returns the value passed to the final Notify() or NotifyRemaining() call. http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@61 PS4, Line 61: returns the value set : /// on 'promise_' in Notify() or Notif same -- To view, visit http://gerrit.cloudera.org:8080/10827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e Gerrit-Change-Number: 10827 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 28 Jun 2018 17:35:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7190: Remove unsupported format writer support
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10823 ) Change subject: IMPALA-7190: Remove unsupported format writer support .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10823/1/be/src/exec/hdfs-table-sink.cc File be/src/exec/hdfs-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/10823/1/be/src/exec/hdfs-table-sink.cc@484 PS1, Line 484: } presumably we do this error checking at runtime rather than analysis so that we can insert into new partitions of a table that has old partitions in a format that's supported for reading but not writing. Do we have a test case to demonstrate that this works? i.e. the case that shows we don't hit these errors when inserting to a table that has these formats in other partitions. -- To view, visit http://gerrit.cloudera.org:8080/10823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I821dc7495a901f1658daa500daf3791b386c7185 Gerrit-Change-Number: 10823 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 28 Jun 2018 17:28:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 ) Change subject: IMPALA-7215: Implement a templatized CountingBarrier .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h File be/src/util/counting-barrier.h: http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@34 PS3, Line 34: /// Sends one notification, decrementing the number of pending notifications by one. Add something like: If this is the final notifier, sets the value returned to the waiter to 'promise_value' http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@36 PS3, Line 36: int32_t looks like no caller actually uses this return value and so you've annotated them with discard_result(). You could also just make this return void since no one wants the value. I'm fine either way. http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@42 PS3, Line 42: /// Sets the number of pending notifications to 0 and unblocks Wait(). similarly, add comment about 'promise_value' http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@54 PS3, Line 54: /// Blocks until all notifications are received. comment the return value http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@55 PS3, Line 55: T would be good to return "const T&" here and below (to match Promise.Get()) http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@58 PS3, Line 58: /// case '*timed_out' will be true. comment return value -- To view, visit http://gerrit.cloudera.org:8080/10827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e Gerrit-Change-Number: 10827 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 28 Jun 2018 04:32:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10815 ) Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. Patch Set 6: Code-Review+1 Fix whitespace glitch, carry. -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 27 Jun 2018 23:39:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Hello Lars Volker, Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10815 to look at the new patch set (#6). Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated Otherwise, if the coordinator to backend CancelFInstances() RPC had failed, the query can hang (and/or finstances can continue running until the query is closed. Testing: - the modified test reproduces the hang without the impalad fix Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M tests/query_test/test_cancellation.py 4 files changed, 51 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/6 -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10815 ) Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. Patch Set 5: Code-Review+1 (2 comments) Carry Lars' +1 http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@366 PS1, Line 366: > Thanks for the explanation. I think the code could be more readable if we r Agree, done. http://gerrit.cloudera.org:8080/#/c/10815/4/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/10815/4/tests/query_test/test_cancellation.py@136 PS4, Line 136: "|".join(filter > remove one "debug_action = " Oops, done. -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 27 Jun 2018 23:37:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Hello Lars Volker, Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10815 to look at the new patch set (#5). Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated Otherwise, if the coordinator to backend CancelFInstances() RPC had failed, the query can hang (and/or finstances can continue running until the query is closed. Testing: - the modified test reproduces the hang without the impalad fix Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M tests/query_test/test_cancellation.py 4 files changed, 51 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/5 -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Hello Lars Volker, Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10815 to look at the new patch set (#4). Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated Otherwise, if the coordinator to backend CancelFInstances() RPC had failed, the query can hang (and/or finstances can continue running until the query is closed. Testing: - the modified test reproduces the hang without the impalad fix Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M tests/query_test/test_cancellation.py 4 files changed, 49 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/4 -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10815 )
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED
if query execution has terminated
..
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@366
PS1, Line 366: rpc_status =
DebugAction(query_ctx.client_request.query_options,
> Wouldn't we need to break; the loop if this was not ok()?
No, I wanted to simulate what happens when a single RPC fails. That way we can
even exercise the retry logic with e.g.
COORD_CANCEL_QUERY_FINSTANCES_RPC:FAIL@0.3
In the test for failing the RPC altogether, I give it a failure probability of
100%, so it loops 3 times and then gives up, just like what would happen if the
RPC really failed.
http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@367
PS1, Line 367: COORD_CANCEL_QUERY_FINSTANCES_RPC
> Should we name this actions so that it's clear that it's in the sender side
The convention I've been using for the global debug action labels is to prefix
with the containing module name. So, I think the "COORD" prefix is sufficient
for knowing this is the sender side. If we want to add one to the handler, it'd
probably be called QUERYSTATE_CANCEL_QUERY_FINSTANCES_RPC. LMK if you feel it's
still not clear.
http://gerrit.cloudera.org:8080/#/c/10815/1/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/10815/1/tests/query_test/test_cancellation.py@139
PS1, Line 139: if fail_rpc_action != None:
> If wait_action != None and fail_rpc_action == None, then debug_action will
Sure, if you feel that's more readable to a native python speaker. (I'd have to
try it out to know what it's doing but I don't consider myself the most
proficient python coder). Note that the extraneous "|" was harmless, but happy
to change this, it does seem more python-y
http://gerrit.cloudera.org:8080/#/c/10815/1/tests/query_test/test_cancellation.py@206
PS1, Line 206: if wait_action is None and fail_rpc_action is None \
> Then this could be "if not debug_action and ('count'..."
Done
--
To view, visit http://gerrit.cloudera.org:8080/10815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Hecht
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Wed, 27 Jun 2018 22:24:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Hello Lars Volker, Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10815 to look at the new patch set (#3). Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated Otherwise, if the coordinator to backend CancelFInstances() RPC had failed, the query can hang (and/or finstances can continue running until the query is closed. Testing: - the modified test reproduces the hang without the impalad fix Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M tests/query_test/test_cancellation.py 4 files changed, 50 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/3 -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] WIP IMPALA-7178: Add the possibility to reduce logging for common data errors
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10793 ) Change subject: WIP IMPALA-7178: Add the possibility to reduce logging for common data errors .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG@22 PS1, Line 22: fragments > My problem with changing RuntimeState::LogError() to log only the first err So it sounds like the motivation is to be able to define a "scope" for what is consider to be redundant errors? I agree doing that in LogError() would not be straightforward. My main concern with building more logic on top of LogError() is that the error log stuff seems a bit broken and so adding more complication on top is hard to reason about. Specifically, the use of max_errors query option seems inconsistent. So wondering if we need to step back and rethink how the warning collection works generally rather than adding another layer on top. Anyway, I'm okay with adding this scoped warning collector thingy if you feel it's needed but it needs more explanation in the class comment. http://gerrit.cloudera.org:8080/#/c/10793/1/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/10793/1/be/src/runtime/runtime-state.h@234 PS1, Line 234: /// when a new error arrives or when errors need to be flushed. this comment needs more explanation. With the current comment, it's not easy for someone to determine whether they should use this class or just call LogError() directly. You should explain how this differs from just calling LogError(). http://gerrit.cloudera.org:8080/#/c/10793/1/be/src/runtime/runtime-state.h@241 PS1, Line 241: if there is no error isn't it: return true if there is already an error with this error code? -- To view, visit http://gerrit.cloudera.org:8080/10793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3b7c1fd020a7ba5e0d9c619e1b67236dce198aa Gerrit-Change-Number: 10793 Gerrit-PatchSet: 1 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 27 Jun 2018 16:46:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h
File be/src/util/counting-barrier.h:
http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h@39
PS1, Line 39: )
rather than have a default_promise_value_, why not just pass it as a parameter
to Notify()? We'll inevitably need that for the typed Promise case.
http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h@46
PS1, Line 46: T* optional_promise_override = nullptr) {
: T promise_value = (optional_promise_override != nullptr) ?
: *optional_promise_override : default_promise_value_
this would be simpler if we just require the promise value for the non-bool
case because otherwise why would you specify the type if you don't want to
specify the value?
Also, rather than making all the callers deal with the meaningless bool arg and
discarding the return value, it seems best to hide the bool case from the
callers (so in the standard case, arg and return values aren't needed since
they are just implementation artifacts). That is, in the places we currently
have a CountingBarrier, the fact that it's implemented with a bool type is just
an implementation artifact. it could have just as well been int or whatever
type, so the users of the plain CountingBarrier shouldn't be aware of it.
One option is to rename this class to TypedCounterBarrier (or whatever name)
and then define CountingBarrier to be a thin wrapper class containing a
TypedCounterBarrier that removes the args/return values to get the
interface back to the status quo for plain CountingBarrier.
--
To view, visit http://gerrit.cloudera.org:8080/10827
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil
Gerrit-Reviewer: Dan Hecht
Gerrit-Comment-Date: Wed, 27 Jun 2018 04:31:29 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10815 )
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED
if query execution has terminated
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10815/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/10815/2/be/src/runtime/coordinator-backend-state.h@262
PS2, Line 262:
I'll remove that whitespace before committing.
http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@336
PS1, Line 336: ) {
> This unfortunately makes the code look a little less readable. Since we req
I'm curious why you feel this is less readable.
I tend to think that passing a parameter for dependencies is more readable for
the following reason: As you know, impala has a history of having bugs due to
object lifetime mismatch issues. These issues arose when objects (of differing
lifetimes) were linked together via pointers. That creates implicit
dependencies of lifetimes that is hard to reason about.
This can be avoided by expressing the dependencies of code explicitly by
passing function arguments. It's obvious that the caller of these functions
must pass a TQueryCtx with sufficient lifetime for this call.
We've also been systematically addressing the lifetime issue by simplifying and
making consistent the lifetime of control structures. For the backend executor
side, that means all query control structures are owned (and have the same
lifetime as) the QueryState. For the coordinator side, we haven't gotten as far
on the cleanup but we should (and it'd likely be the ClientRequestState
lifetime, once we clean that class up).
The other problem with creating links between objects is that it makes it easy
to be lazy about defining the right abstractions, which e.g. leads to
bi-direction calls between objects (i.e spaghetti code).
Also, I don't want to copy the TQueryCtx into each BackendState because then it
becomes less clear that this is a query-wide structure and the copies per
backend are a waste of memory (though maybe you meant copy a pointer to it?).
All that said, given the tight relationship between this class and the
Coordinator (and the fact that the Coordinator owns this), I'm okay with
keeping a back pointer to the coordinator to access the query metadata. I think
both patchsets are okay, but I assume you prefer the second. LMK if not.
--
To view, visit http://gerrit.cloudera.org:8080/10815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Hecht
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Comment-Date: Wed, 27 Jun 2018 04:07:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10815 to look at the new patch set (#2). Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated Otherwise, if the coordinator to backend CancelFInstances() RPC had failed, the query can hang (and/or finstances can continue running until the query is closed. Testing: - the modified test reproduces the hang without the impalad fix Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M tests/query_test/test_cancellation.py 4 files changed, 54 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/2 -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 ) Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode .. Patch Set 6: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc@310 PS4, Line 310:*node = pool->Add(new AggregationNode(pool, tnode, descs)); : } > I added this because its generally nice to have safety valves in case of re Thanks for removing it. http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.h File be/src/exec/grouping-aggregator.h: http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.h@130 PS4, Line 130: /// streamed through and returned in 'out_batch'. AddBatch() and AddBatchStreaming() > I think it would be fine, but since there's no reason to and its not tested Okay. i'm fine either way - I just wasn't sure if it was a requirement or not. http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.cc File be/src/exec/grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.cc@91 PS4, Line 91: is_streaming_preagg_(tnode.agg_node.use_streaming_preaggregation), > Yes but a GroupingAggregator can be used with either the StreamingAggNode o Right, I was imaging that eventually we'll have separate thrift nodes for AggNode and StreamingAggNode, at which time we'd get rid of this thrift boolean and instead the constructor would just take a boolean constant depending on the callsite. But maybe we don't plan to have the same class structure in the frontend and in thrift? -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 27 Jun 2018 03:25:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7163: Implement a state machine for the QueryState class
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10813 )
Change subject: IMPALA-7163: Implement a state machine for the QueryState class
..
Patch Set 2:
(12 comments)
Some initial high level comments on the states and the synchronization.
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@66
PS2, Line 66: f any fragment instance hits an error or cancellation, then we
immediately change the
: /// state of the query to the corresponding error state.
but won't we need to wait until all FIS have completed (even if it's because
they were cancelled when the first FIS failed) before sending the final report?
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@194
PS2, Line 194: pair
rather than using a pair<>, how about defining a struct so that the fields have
meaningful names?
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@245
PS2, Line 245: 2&
we generally only use const references. For things that will be modified, use a
pointer so it's obvious at the callsite.
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@245
PS2, Line 245: pair
Use FInstanceStatus
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@262
PS2, Line 262: to another thread that
: // called this function.
That doesn't seem right in the "less than 0" case, since this function only
ever sets the count to 0. I think the less than case requires racing with both
this function and DoneWithState(). Anyway, I think these details all go away if
you can use the CountingBarrier<> idea.
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@289
PS2, Line 289: INITIALIZED,
: /// STARTED: All fragment instances have been started.
Implies that the query is
: /// being prepared.
: STARTED,
: /// PREPARED: All fragment instances managed by this
QueryState have successfully
: /// completed Prepare(). Implies that the query is Opening.
: PREPARED,
: /// OPENED: All fragment instances managed by this
QueryState have successfully
: /// completed Open(). Implies that the query is executing.
: OPENED,
why do we need all of these states? it's a bit hard to see how they will be
used when the code to use them isn't here yet. could you at least explain that
or reduce the number of states in this patch (and introduce them back later as
needed)?
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@300
PS2, Line 300: The QueryState is ready for tear-down at this state
what about the other terminal states? when is the QueryState ready for teardown
in those cases?
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@312
PS2, Line 312: only expect a single thread to call this function.
that's the defintion of not thread safe :) It be more helpful to explain what
mechanism guarantees that this is only called by a single thread. i.e. this is
a helper to MonitorFInstances() presumably, and up there document that that
thing should be called by a single thread.
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@337
PS2, Line 337: Is
The "Is" sounds like it returns a bool, yet the return type is void. What does
"validate" mean? If this means DCHECK, just call it
DCheckLegalExecStateTransition()
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.h@344
PS2, Line 344: AtomicInt32 num_remaining_finstances_preparing_;
: AtomicInt32 num_remaining_finstances_opening_;
: AtomicInt32 num_remaining_finstances_executing_;
how are these going to be used? it seems like we'd want them to be
CountingBarriers (that the monitoring thread will do timed blocks on). Oh, I
guess we'll use the Promises to do that.
But then, it seems what we really want is to have a CountingBarrier where
our existing CountingBarrier is CountingBarrier and this code uses
CountingBarrier.
Then, I think your DoneWithState() is just CountingBarrier::Notify(OK) and
ErrorDuringState is CountingBarrier::NotifyRemaining(fis_error).
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/10813/2/be/src/runtime/query-state.cc@437
PS2, Line 437:
: if (!prepare_status.first.ok()) {
: // If 'prepare_status' is not OK, then one of the fragment
instances has hit an error.
: // Don't return until every instance is prepared and record
the first non-OK
: // (non-CANCELLED if available) status.
[Impala-ASF-CR] IMPALA-7210: global debug actions should be case insensitive
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10814 ) Change subject: IMPALA-7210: global debug actions should be case insensitive .. Patch Set 3: Code-Review+2 Carry -- To view, visit http://gerrit.cloudera.org:8080/10814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3f738caeb602afce4ca638ce354302e521187dc Gerrit-Change-Number: 10814 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 26 Jun 2018 15:36:18 + Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-7178: Add the possibility to reduce logging for common data errors
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10793 ) Change subject: WIP IMPALA-7178: Add the possibility to reduce logging for common data errors .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG@12 PS1, Line 12: the logs : will contain a new line for every occurrence > I was hesitant to go that way, because messages with the same error code bu But isn't that the point of the change (as you state below). I guess it would help if you could summarize which cases you feel the logging is useful to preserve (even when the errors are aggregated) and which you feel the logging should be eliminated. http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG@22 PS1, Line 22: fragments > I agree that this logic would be mainly useful for scanners. KuduSink has As the class is written today, it's not really tied to RuntimeState. You could put it anywhere, right? But, it's still not clear to me what the rule of thumb is for when developers should use this new class vs. just calling SetError() directly going forward (if, as you say, sometimes the individual logging is useful). If the primary motivation is to deal with the logging, why not just do something simpler, rather than introduce a whole class, like pass a boolean to SetError() (or add a parallel SetError() interface) that only logs the first time that error code is encountered (but doesn't introduce extra state)? -- To view, visit http://gerrit.cloudera.org:8080/10793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3b7c1fd020a7ba5e0d9c619e1b67236dce198aa Gerrit-Change-Number: 10793 Gerrit-PatchSet: 1 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 26 Jun 2018 15:32:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10394 )
Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc@310
PS4, Line 310: if (FLAGS_use_legacy_aggregation) {
: *node = pool->Add(new PartitionedAggregationNode(pool,
tnode, descs));
> i guess we need to at least keep it until the frontend change is in to use
Oops, ignore the previous comment. I wrote it without re-reading this code,
which obviously generates the new exec nodes even without the frontend changes,
which is okay for now.
--
To view, visit http://gerrit.cloudera.org:8080/10394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e
Gerrit-Change-Number: 10394
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 26 Jun 2018 15:22:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Revert "IMPALA-5893: Remove old kinit code for Impala 3"
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10563 ) Change subject: Revert "IMPALA-5893: Remove old kinit code for Impala 3" .. Patch Set 1: Do we have a plan for addressing IMPALA-7072 and removing this code again? Or do we think we need to keep this code around forever (which would be unfortunate given the fork() problem)? -- To view, visit http://gerrit.cloudera.org:8080/10563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbcce96a1fab23445f42d224ca397897aa90e07d Gerrit-Change-Number: 10563 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 26 Jun 2018 00:31:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10394 )
Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode
..
Patch Set 4: Code-Review+1
(3 comments)
This all seems pretty good to me. Tim should take a pass as well.
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc@310
PS4, Line 310: if (FLAGS_use_legacy_aggregation) {
: *node = pool->Add(new PartitionedAggregationNode(pool,
tnode, descs));
> do we need to keep this code around?
i guess we need to at least keep it until the frontend change is in to use the
new exec nodes. Do we have a change that does that without changing
functionality, or is it all bundled with the multiple distinct frontend changes?
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.h
File be/src/exec/grouping-aggregator.h:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.h@130
PS4, Line 130: /// streamed through and returned in 'out_batch'.
is it a rule that AddBatchStreaming() and AddBatch() shouldn't both be used on
the same aggregator?
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.cc
File be/src/exec/grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/grouping-aggregator.cc@91
PS4, Line 91: is_streaming_preagg_(tnode.agg_node.use_streaming_preaggregation),
should this be implied by the exec node type, now that we have a distinct exec
node for pre-agg?
--
To view, visit http://gerrit.cloudera.org:8080/10394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e
Gerrit-Change-Number: 10394
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 26 Jun 2018 00:28:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6425: reduce MemPool max chunk size
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10789 ) Change subject: IMPALA-6425: reduce MemPool max chunk size .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10789/1/be/src/runtime/mem-pool.h File be/src/runtime/mem-pool.h: http://gerrit.cloudera.org:8080/#/c/10789/1/be/src/runtime/mem-pool.h@182 PS1, Line 182: /// a freelist in TCMalloc's central cache. > I looked at the TCMalloc code and it seem to be kMaxSize= 256 * 1024, a I wonder if it's better for mem pool to come from global or thread lists. Anyway, that's a separate question that we don't need to solve now. -- To view, visit http://gerrit.cloudera.org:8080/10789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58cc3031ce592892cb9efe9ab41f07d86468b08c Gerrit-Change-Number: 10789 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 26 Jun 2018 00:13:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10815 Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated .. IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated Otherwise, if the coordinator to backend CancelFInstances() RPC had failed, the query can hang (and/or finstances can continue running until the query is closed. Testing: - the modified test reproduces the hang without the impalad fix Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M tests/query_test/test_cancellation.py 4 files changed, 34 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/1 -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] IMPALA-6425: reduce MemPool max chunk size
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10789 ) Change subject: IMPALA-6425: reduce MemPool max chunk size .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10789/1/be/src/runtime/mem-pool.h File be/src/runtime/mem-pool.h: http://gerrit.cloudera.org:8080/#/c/10789/1/be/src/runtime/mem-pool.h@182 PS1, Line 182: /// a freelist in TCMalloc's central cache. At what size do allocations come from a thread-local list? -- To view, visit http://gerrit.cloudera.org:8080/10789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58cc3031ce592892cb9efe9ab41f07d86468b08c Gerrit-Change-Number: 10789 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Mon, 25 Jun 2018 22:48:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-7178: Add the possibility to reduce logging for common data errors
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10793 ) Change subject: WIP IMPALA-7178: Add the possibility to reduce logging for common data errors .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG@22 PS1, Line 22: fragments > The join build thread is the other case to keep in mind. Right, but I don't think we do much LogError() from that (other than from inside the scan node), so it doesn't seem like it really contributes to lock contention. -- To view, visit http://gerrit.cloudera.org:8080/10793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3b7c1fd020a7ba5e0d9c619e1b67236dce198aa Gerrit-Change-Number: 10793 Gerrit-PatchSet: 1 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 25 Jun 2018 22:32:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-7178: Add the possibility to reduce logging for common data errors
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10793 ) Change subject: WIP IMPALA-7178: Add the possibility to reduce logging for common data errors .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG@12 PS1, Line 12: the logs : will contain a new line for every occurrence seems like we could change that generally. http://gerrit.cloudera.org:8080/#/c/10793/1//COMMIT_MSG@22 PS1, Line 22: fragments RuntimeState is per fragment instance state. It's not shared between fragments or even instances. In that regard, it's already the per-thread state for most of execution. The scanners are currently (with mt_dop=0) the important exception. So then, what is the primary reason to add the new class? Is it to avoid the locking or avoid the log spew. For the former, perhaps this new class would be better to live somewhere specific to the scanners (otherwise, I think it's unclear whether code should use this class or call directly to the LogError() interface). For the latter, we can additionally consider fixing LogError() to avoid the redundant logs (note that most callers of LogError() are the scanners especially in cases that will produce many messages). -- To view, visit http://gerrit.cloudera.org:8080/10793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3b7c1fd020a7ba5e0d9c619e1b67236dce198aa Gerrit-Change-Number: 10793 Gerrit-PatchSet: 1 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 25 Jun 2018 22:16:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7210: global debug actions should be case insensitive
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10814 Change subject: IMPALA-7210: global debug actions should be case insensitive .. IMPALA-7210: global debug actions should be case insensitive The ExecNode debug actions don't care about case so better to be consistent. Testing: verify that this works: set debug_action=coord_before_exec_rpc:sleep@1000 Change-Id: Ia3f738caeb602afce4ca638ce354302e521187dc --- M be/src/util/debug-util.cc 1 file changed, 7 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/10814/1 -- To view, visit http://gerrit.cloudera.org:8080/10814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia3f738caeb602afce4ca638ce354302e521187dc Gerrit-Change-Number: 10814 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h@278 PS2, Line 278: exec_state_ > exec_state_ Done http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h@279 PS2, Line 279: both : // fields > does this mean reading both fields atomically? Wording is a little unclear. let me know if that clarifies, or if not what might help. -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 25 Jun 2018 18:54:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Hello Sailesh Mukil, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10811 to look at the new patch set (#3). Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 19 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/10811/3 -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/common/atomic.h File be/src/common/atomic.h: http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/common/atomic.h@156 PS1, Line 156: static_cast > Since they're enums, shouldn't implicit casts work? No, because it's an "enum class" and so no implicit conversions. http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.cc@a511 PS2, Line 511: : : : > I've not done an analysis, but is it fair to say that using an atomic enum For x86, Load() and Store() only needs to ensure that the memory access emitted by the compiler occurs with a single instruction (and prevent compiler reordering). For x86, the CPU always ensures that (aligned) loads or stores are atomic. i.e. no lck prefix is needed. (See atomicops-internals-x86.h which is what it ultimately boils down to.) So, should be strictly better since there is no real runtime overhead on x86 to using atomic load/store. (That wouldn't necessarily be true if it was load-modify-write, i.e. cmpxchg). -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 25 Jun 2018 18:45:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h@278 PS1, Line 278: exec_status_. exec_state_ can be read i > update that. Done -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 25 Jun 2018 18:13:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 19 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/10811/2 -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h@278 PS1, Line 278: // protects exec-state_ and exec_status_ update that. -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 25 Jun 2018 18:13:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10811 Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 15 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/10811/1 -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10394 )
Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode
..
Patch Set 4:
(5 comments)
I still need to look at grouping-aggregator-* one more time but here's the
comments for the rest.
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/exec-node.cc@310
PS4, Line 310: if (FLAGS_use_legacy_aggregation) {
: *node = pool->Add(new PartitionedAggregationNode(pool,
tnode, descs));
do we need to keep this code around?
it seems like the new stuff should be functionally, algorithmically, and
performance equivalent, so how about we just delete the old code? it's hard to
test both code paths and the risk here seems low enough that we can just delete
the old one.
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/non-grouping-aggregator.h
File be/src/exec/non-grouping-aggregator.h:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/non-grouping-aggregator.h@103
PS4, Line 103: 'process_batch_fn_' or
: /// 'process_batch_no_grouping_fn_' will be updated with the
codegened function
: /// depending on whether this is a grouping or non-grouping
aggregation.
is that still accurate?
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/non-grouping-aggregator.h@107
PS4, Line 107: CodegenProcessBatch
also the naming seems inconsistent with ProcessBatchNoGrouping(). The
NoGrouping part seems redundant with the class name now, but I don't mind
either way if we can make it consistent with the CodegenFn naming.
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/non-grouping-aggregator.cc
File be/src/exec/non-grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/non-grouping-aggregator.cc@123
PS4, Line 123: ProcessBatchNoGrouping
given that this is no 1:1 with AddBatch(), maybe just call it AddBatchWork() to
make that obvious (and do the renaming for codegen fn/vars)?
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/streaming-aggregation-node.h
File be/src/exec/streaming-aggregation-node.h:
http://gerrit.cloudera.org:8080/#/c/10394/4/be/src/exec/streaming-aggregation-node.h@35
PS4, Line 35: f there is not enough memory available
I think it also stops aggregating if it's not seeing a good reduction? if so,
maybe clarify with that.
--
To view, visit http://gerrit.cloudera.org:8080/10394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e
Gerrit-Change-Number: 10394
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 22 Jun 2018 21:56:41 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] [DRAFT] IMPALA-6189: Add thread watchdogs for HDFS IO calls
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10696 ) Change subject: [DRAFT] IMPALA-6189: Add thread watchdogs for HDFS IO calls .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10696/3/be/src/runtime/io/scan-range.cc File be/src/runtime/io/scan-range.cc: http://gerrit.cloudera.org:8080/#/c/10696/3/be/src/runtime/io/scan-range.cc@632 PS3, Line 632: fs_, hdfs_file, position_in_file as mentioned before, we'll want to capture the argument values since that will likely be the first question some asks after seeing the callstack. if you don't want to do it in this patch, could you please file a follow on JIRA so we don't forget? -- To view, visit http://gerrit.cloudera.org:8080/10696 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28e918761c120043d332b034450208eaf34e3e2b Gerrit-Change-Number: 10696 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zoram Thanga Gerrit-Comment-Date: Fri, 22 Jun 2018 16:37:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5202: Disallow PREPARE:WAIT debug action
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10776 ) Change subject: IMPALA-5202: Disallow PREPARE:WAIT debug action .. Patch Set 2: Code-Review+2 Rebased onto master -- To view, visit http://gerrit.cloudera.org:8080/10776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1caa4f8e6ce7f32a8a3722648e08e24f34dba35d Gerrit-Change-Number: 10776 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 21 Jun 2018 04:03:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 ) Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h File be/src/exec/aggregator.h: http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h@54 PS3, Line 54: AggregationNode Used for other exec nodes, no? (e.g. streaming) http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h@63 PS3, Line 63: virtual Status Init(const TPlanNode& tnode, RuntimeState* state) WARN_UNUSED_RESULT; : virtual Status Prepare(RuntimeState* state) WARN_UNUSED_RESULT; : virtual void Codegen(RuntimeState* state) = 0; : virtual Status Open(RuntimeState* state) WARN_UNUSED_RESULT; : virtual Status GetNext( : RuntimeState* state, RowBatch* row_batch, bool* eos) WARN_UNUSED_RESULT = 0; : virtual Status Reset(RuntimeState* state) WARN_UNUSED_RESULT = 0; : virtual void Close(RuntimeState* state); I guess these are used one-to-one to implement the ExecNode interface? Or are there any subtle differences in lifecycle? let's briefly comment either way. http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h@80 PS3, Line 80: SetDebugOptions why does the aggregator need to copy the debug_options? where does it get used at the aggregator level? http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/streaming-aggregation-node.h File be/src/exec/streaming-aggregation-node.h: http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/streaming-aggregation-node.h@31 PS3, Line 31: Node for doing partitioned hash aggregation. : /// This node consumes the input from child(0) during GetNext() and then passes it to the : /// Aggregator, which does the actual work of aggregating. : /// This node only supports grouping aggregations. somewhere in here we should explain how this differs from AggregationNode, i.e. what does streaming mean. -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 21 Jun 2018 00:21:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7191: don't call srand() at random times
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10778 ) Change subject: IMPALA-7191: don't call srand() at random times .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10778/2/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/10778/2/be/src/runtime/krpc-data-stream-sender.cc@a604 PS2, Line 604: > Not sure if it warrants a comment that the random number generator is seede I don't think so but I can add it if you prefer. -- To view, visit http://gerrit.cloudera.org:8080/10778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f Gerrit-Change-Number: 10778 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Jun 2018 22:57:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10690 )
Change subject: IMPALA-7046: introduce "global" debug_actions
..
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10690/13/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/10690/13/be/src/util/debug-util.cc@296
PS13, Line 296: split(actions, debug_actions, is_any_of("|"),
token_compress_on);
> Apologies for being late. We recently had a couple of ASAN builds fail when
I have have doubts that split() was the reason problem there, and especially
that it could be a problem here. So I think this is fine to keep.
--
To view, visit http://gerrit.cloudera.org:8080/10690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388
Gerrit-Change-Number: 10690
Gerrit-PatchSet: 13
Gerrit-Owner: Dan Hecht
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 20 Jun 2018 22:50:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7191: don't call srand() at random times
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10778 ) Change subject: IMPALA-7191: don't call srand() at random times .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10778/1/be/src/common/init.cc File be/src/common/init.cc: http://gerrit.cloudera.org:8080/#/c/10778/1/be/src/common/init.cc@176 PS1, Line 176: srand(time(NULL)); > Would it be better to use random_device here? I don't think it matters. If we require more randomness than this then we probably shouldn't be using rand() in those places to begin with. -- To view, visit http://gerrit.cloudera.org:8080/10778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f Gerrit-Change-Number: 10778 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Jun 2018 22:48:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7191: don't call srand() at random times
Hello Michael Ho, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10778 to look at the new patch set (#2). Change subject: IMPALA-7191: don't call srand() at random times .. IMPALA-7191: don't call srand() at random times Instead, call it from the daemons' main(), and remove the calls that happen at random time during the lifetime of the daemons. Doesn't attempt to cleanup the backend unit tests which can happen separately or not at all. Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f --- M be/src/common/init.cc M be/src/exec/hdfs-table-sink.cc M be/src/runtime/data-stream-sender.cc M be/src/runtime/krpc-data-stream-sender.cc M be/src/testutil/fault-injection-util.cc 5 files changed, 2 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/10778/2 -- To view, visit http://gerrit.cloudera.org:8080/10778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f Gerrit-Change-Number: 10778 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7191: don't call srand() at random times
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10778 ) Change subject: IMPALA-7191: don't call srand() at random times .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10778/1/be/src/util/network-util.cc File be/src/util/network-util.cc: http://gerrit.cloudera.org:8080/#/c/10778/1/be/src/util/network-util.cc@a186 PS1, Line 186: > oops, I meant to revert that since not trying to deal with testing code. Done -- To view, visit http://gerrit.cloudera.org:8080/10778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f Gerrit-Change-Number: 10778 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Jun 2018 22:38:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7191: don't call srand() at random times
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10778 ) Change subject: IMPALA-7191: don't call srand() at random times .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10778/1/be/src/util/network-util.cc File be/src/util/network-util.cc: http://gerrit.cloudera.org:8080/#/c/10778/1/be/src/util/network-util.cc@a186 PS1, Line 186: oops, I meant to revert that since not trying to deal with testing code. -- To view, visit http://gerrit.cloudera.org:8080/10778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f Gerrit-Change-Number: 10778 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Jun 2018 22:36:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7191: don't call srand() at random times
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10778 Change subject: IMPALA-7191: don't call srand() at random times .. IMPALA-7191: don't call srand() at random times Instead, call it from the daemons' main(), and remove the calls that happen at random time during the lifetime of the daemons. Doesn't attempt to cleanup the backend unit tests which can happen separately or not at all. Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f --- M be/src/common/init.cc M be/src/exec/hdfs-table-sink.cc M be/src/runtime/data-stream-sender.cc M be/src/runtime/krpc-data-stream-sender.cc M be/src/testutil/fault-injection-util.cc M be/src/util/network-util.cc 6 files changed, 2 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/10778/1 -- To view, visit http://gerrit.cloudera.org:8080/10778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iaf0ef5a0e842ebdcdb6d7355302e68fb0bc7ef5f Gerrit-Change-Number: 10778 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] [WIP] Skip remaining exec rpcs if a backend has failed
Dan Hecht has abandoned this change. ( http://gerrit.cloudera.org:8080/10691 ) Change subject: [WIP] Skip remaining exec rpcs if a backend has failed .. Abandoned Oops, didn't mean to publish this. -- To view, visit http://gerrit.cloudera.org:8080/10691 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Idf80c1dd7c651901bae0ba6ecb555d7a95bcbd10 Gerrit-Change-Number: 10691 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] [WIP] Skip remaining exec rpcs if a backend has failed
Dan Hecht has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10691 ) Change subject: [WIP] Skip remaining exec rpcs if a backend has failed .. [WIP] Skip remaining exec rpcs if a backend has failed Change-Id: Idf80c1dd7c651901bae0ba6ecb555d7a95bcbd10 --- M be/src/common/atomic.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 6 files changed, 68 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/10691/5 -- To view, visit http://gerrit.cloudera.org:8080/10691 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idf80c1dd7c651901bae0ba6ecb555d7a95bcbd10 Gerrit-Change-Number: 10691 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] IMPALA-5202: Disallow PREPARE:WAIT debug action
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10776 Change subject: IMPALA-5202: Disallow PREPARE:WAIT debug action .. IMPALA-5202: Disallow PREPARE:WAIT debug action In order to simplify FIS startup, we don't allow cancellation until all FIS have finished Prepare(), so we shouldn't allow PREPARE:WAIT since there will be no way to cancel out of the loop. Make this explicit. Change-Id: I1caa4f8e6ce7f32a8a3722648e08e24f34dba35d --- M be/src/exec/exec-node.cc M be/src/runtime/debug-options.cc M tests/failure/test_failpoints.py 3 files changed, 15 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/10776/1 -- To view, visit http://gerrit.cloudera.org:8080/10776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1caa4f8e6ce7f32a8a3722648e08e24f34dba35d Gerrit-Change-Number: 10776 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 ) Change subject: IMPALA-7046: introduce "global" debug_actions .. Patch Set 11: Code-Review+2 Carry +2, fix obvious missing reference found by clang-tidy -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 11 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Jun 2018 16:16:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Hello Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10690 to look at the new patch set (#11). Change subject: IMPALA-7046: introduce "global" debug_actions .. IMPALA-7046: introduce "global" debug_actions The motivation is to add jitter to backend startup in test_failpoints. The race in IMPALA-7033 can be reproduced by adding jitter to the exec rpcs when some backends fail. Let's add jitter to test_failpoints to get better coverage of exec startup races. This builds on top of the debug action extensions added in the async admission control patch by allowing the new "global" debug actions (i.e. actions that can be used in points outside of the ExecNodes). See the code comments for details. For now, we're only using the SLEEP and JITTER commands, but I've included a FAIL command as well since I'll want to use that to write a test for IMPALA-6788 to simulate exec rpc failure. Note that I don't bother resolving the actions ahead of time (like we do for ExecNode actions). It doesn't seem worth it since the resolution only needs to occur after we've matched the label and I don't expect the same label to be hit many times within a single thread. We can always optimize this later if needed. Testing: - Verified that test_failpoints can reproduce the race in IMPALA-7033 by reverting that fix and testing. - Ran the modified tests and grepped the impalad log to see that the sleeps are still occuring. - Manually verify global FAIL command (in a build with another patch). - Manually verified invalid debug_actions (both ExecNode and global) Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 --- M be/src/runtime/coordinator.cc M be/src/runtime/debug-options.cc M be/src/scheduling/admission-controller.cc M be/src/service/client-request-state.cc M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_admission_controller.py M tests/failure/test_failpoints.py M tests/query_test/test_observability.py 10 files changed, 201 insertions(+), 70 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/10690/11 -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 11 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Fail cleanly when in process server can't bind
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10726 ) Change subject: Fail cleanly when in process server can't bind .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10726/1/be/src/testutil/in-process-servers.h File be/src/testutil/in-process-servers.h: http://gerrit.cloudera.org:8080/#/c/10726/1/be/src/testutil/in-process-servers.h@55 PS1, Line 55: /// forwarded to the ExecEnv. can you specify whether *server is set only when OK is returned, or if it might be set even if an error is returned. -- To view, visit http://gerrit.cloudera.org:8080/10726 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I376a2aa559f4b5cf3b96fa3465520e9983ecec4b Gerrit-Change-Number: 10726 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Tue, 19 Jun 2018 21:59:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. (
http://gerrit.cloudera.org:8080/10690 )
Change subject: IMPALA-7046: introduce "global" debug_actions
..
Patch Set 8:
(9 comments)
Thanks for the reviews Tim and Bikram!
http://gerrit.cloudera.org:8080/#/c/10690/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10690/8//COMMIT_MSG@28
PS8, Line 28: of Release builds.
> Your approach to this changed, right?
Done
http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/runtime/debug-options.cc
File be/src/runtime/debug-options.cc:
http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/runtime/debug-options.cc@58
PS6, Line 58: continue
> I think all global debug actions have a size of exactly 2 components (separ
That's true - my initial implementation had three components for global
actions. I'll revert this and add a comment instead.
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/runtime/debug-options.cc
File be/src/runtime/debug-options.cc:
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/runtime/debug-options.cc@60
PS8, Line 60: DCHECK(!(phase_ == TExecNodePhase::CLOSE && action_ ==
TDebugAction::WAIT))
> Maybe we should just log an error here - I don't think we want users to be
Yeah, I was thinking that too but then decided to just copy the logic from old
line 60, since I wanted to return an error status but that's a bit non-trivial
right now. I'll fix it up a bit now to do the logging.
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@295
PS8, Line 295: list
> Any reason to use a list and not a vector? Seems arbitrary but I might be m
Just figured you don't really need random access to the list of debug actions
and there is no ordering of that thing. That's unlikely the components of each
debug action, where you do want random access and ordering is meaningful, so
vector there.
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@318
PS8, Line 318: if (parse_result != StringParser::PARSE_SUCCESS ||
> Should we log a parse failure?
It turns into a status at lines 356/367. Are you looking for something more?
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@323
PS8, Line 323: *should_execute = rand() < probability * (RAND_MAX + 1L);
> We use rand() here and std::mt19937 below. Any reason not to standardise on
Not really. I think I'll change the one below to rand() which is what I meant
to do but forgot (because it seems wrong to create a new random device each
time).
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@331
PS8, Line 331: error_msg
> How about upper case to make it clearer that it's a constant? I was momenta
Done
http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@339
PS8, Line 339: if (cmd.compare("SLEEP") == 0) {
> This is ok, but why not (cmd == "SLEEP")? Dislike for operator overloading?
yeah, I do generally disklike operator overloading, though don't feel too
strongly in this particular case. also carried this from the old code at line
293. I'f you don't feel strongly, I'd prefer to leave it.
http://gerrit.cloudera.org:8080/#/c/10690/8/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/10690/8/common/thrift/ImpalaService.thrift@91
PS8, Line 91: // 2. Global actions
> I think this is fine for now but if the set of things continue to grow I wo
Yeah, let's defer that. I'm not sure there are that many more commands we need.
--
To view, visit http://gerrit.cloudera.org:8080/10690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388
Gerrit-Change-Number: 10690
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Hecht
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 19 Jun 2018 17:51:16 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Hello Tim Armstrong, Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10690 to look at the new patch set (#9). Change subject: IMPALA-7046: introduce "global" debug_actions .. IMPALA-7046: introduce "global" debug_actions The motivation is to add jitter to backend startup in test_failpoints. The race in IMPALA-7033 can be reproduced by adding jitter to the exec rpcs when some backends fail. Let's add jitter to test_failpoints to get better coverage of exec startup races. This builds on top of the debug action extensions added in the async admission control patch by allowing the new "global" debug actions (i.e. actions that can be used in points outside of the ExecNodes). See the code comments for details. For now, we're only using the SLEEP and JITTER commands, but I've included a FAIL command as well since I'll want to use that to write a test for IMPALA-6788 to simulate exec rpc failure. Note that I don't bother resolving the actions ahead of time (like we do for ExecNode actions). It doesn't seem worth it since the resolution only needs to occur after we've matched the label and I don't expect the same label to be hit many times within a single thread. We can always optimize this later if needed. Testing: - Verified that test_failpoints can reproduce the race in IMPALA-7033 by reverting that fix and testing. - Ran the modified tests and grepped the impalad log to see that the sleeps are still occuring. - Manually verify global FAIL command (in a build with another patch). - Manually verified invalid debug_actions (both ExecNode and global) Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 --- M be/src/runtime/coordinator.cc M be/src/runtime/debug-options.cc M be/src/scheduling/admission-controller.cc M be/src/service/client-request-state.cc M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_admission_controller.py M tests/failure/test_failpoints.py M tests/query_test/test_observability.py 10 files changed, 201 insertions(+), 70 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/10690/9 -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 9 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6969: add AC last queued cause to profile
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10731 ) Change subject: IMPALA-6969: add AC last queued cause to profile .. Patch Set 5: Code-Review+2 Bikram, could you take another quick look to make sure you agree with the new wording? -- To view, visit http://gerrit.cloudera.org:8080/10731 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ida9b75dc50dfb7a27f59deda91bad6ac838130a1 Gerrit-Change-Number: 10731 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 19 Jun 2018 00:00:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7115: set a default THREAD RESERVATION LIMIT value
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10628 ) Change subject: IMPALA-7115: set a default THREAD_RESERVATION_LIMIT value .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10628 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I31d3fa3f6305c360922649dba53a9026c9563384 Gerrit-Change-Number: 10628 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 23:34:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6969: add AC last queued cause to profile
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10731 ) Change subject: IMPALA-6969: add AC last queued cause to profile .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10731/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10731/3/be/src/scheduling/admission-controller.cc@110 PS3, Line 110: Admission queue details" > I also had the same concern. I didn't want to change the strings for no goo that sounds pretty good to me (maybe add "admission" in there). -- To view, visit http://gerrit.cloudera.org:8080/10731 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ida9b75dc50dfb7a27f59deda91bad6ac838130a1 Gerrit-Change-Number: 10731 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 23:22:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 ) Change subject: IMPALA-7046: introduce "global" debug_actions .. Patch Set 8: i think this is ready for review again. -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 18 Jun 2018 23:04:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Hello Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10690 to look at the new patch set (#8). Change subject: IMPALA-7046: introduce "global" debug_actions .. IMPALA-7046: introduce "global" debug_actions The motivation is to add jitter to backend startup in test_failpoints. The race in IMPALA-7033 can be reproduced by adding jitter to the exec rpcs when some backends fail. Let's add jitter to test_failpoints to get better coverage of exec startup races. This builds on top of the debug action extensions added in the async admission control patch by allowing the new "global" debug actions (i.e. actions that can be used in points outside of the ExecNodes). See the code comments for details. For now, we're only using the SLEEP and JITTER commands, but I've included a FAIL command as well since I'll want to use that to write a test for IMPALA-6788 to simulate exec rpc failure. Note that I don't bother resolving the actions ahead of time (like we do for ExecNode actions). It doesn't seem worth it since the resolution only needs to occur after we've matched the label and I don't expect the same label to be hit many times within a single thread. We can always optimize this later if needed. Instead, just continue compiling it out of Release builds. Testing: - Verified that test_failpoints can reproduce the race in IMPALA-7033 by reverting that fix and testing. - Ran the modified tests and grepped the impalad log to see that the sleeps are still occuring. - Manually verify global FAIL command (in a build with another patch). Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 --- M be/src/runtime/coordinator.cc M be/src/runtime/debug-options.cc M be/src/scheduling/admission-controller.cc M be/src/service/client-request-state.cc M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_admission_controller.py M tests/failure/test_failpoints.py M tests/query_test/test_observability.py 10 files changed, 191 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/10690/8 -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 ) Change subject: IMPALA-7046: introduce "global" debug_actions .. Patch Set 7: Bikram, Let me finish some testing of this before you review the new patchset. -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 18 Jun 2018 21:55:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 ) Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode .. Patch Set 2: > I think that's probably unnecessary (hopefully this shouldn't be > too terrible to review, since the logic is entirely unchanged), But it sounds like there is some changes in here that aren't purely code motion. i.e. some cleanups are in here too. so what i'm really wondering is how can we focus the review on those parts? Maybe you just want to add comments to indicate which code isn't a straight copy (and class rename)? Or some other way to highlight the more interesting parts? -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Mon, 18 Jun 2018 21:19:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6969: add AC last queued cause to profile
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10731 ) Change subject: IMPALA-6969: add AC last queued cause to profile .. Patch Set 3: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/10731/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10731/3/be/src/scheduling/admission-controller.cc@110 PS3, Line 110: Admission queue details" so the reason given in this entry will always be the initial reason, is that right? Will it be clear what this entry means vs the new entry? http://gerrit.cloudera.org:8080/#/c/10731/3/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/10731/3/tests/custom_cluster/test_admission_controller.py@604 PS3, Line 604: EXPECTED_CAUSE = \ for these tests, do you think it's also worth verifying that the initial reason is as expected? -- To view, visit http://gerrit.cloudera.org:8080/10731 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ida9b75dc50dfb7a27f59deda91bad6ac838130a1 Gerrit-Change-Number: 10731 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 21:16:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 21:05:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 ) Change subject: IMPALA-7046: introduce "global" debug_actions .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.cc@374 PS6, Line 374: 2 > that should be 1 (I messed it up when refactoring). Done -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 18 Jun 2018 20:53:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 ) Change subject: IMPALA-7046: introduce "global" debug_actions .. Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/runtime/debug-options.cc File be/src/runtime/debug-options.cc: http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/runtime/debug-options.cc@58 PS6, Line 58: continue > should we just return here instead(and on L55), since we don't expect this I was thinking it could be possible for a global debug action to happen to match up to here, so we should keep looking for the execnode action. http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.cc@310 PS6, Line 310: DebugActionImpl() > nit: DebugActionImpl Done http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.cc@335 PS6, Line 335: CK_GE(tokens.size > this wont work with "FAIL" given no probability Oops, messed that up when refactoring. Will fix and restest. http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.cc@374 PS6, Line 374: that should be 1 (I messed it up when refactoring). http://gerrit.cloudera.org:8080/#/c/10690/6/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/10690/6/common/thrift/ImpalaService.thrift@95 PS6, Line 95: SLEEP@ > nit: SLEEP@ Done http://gerrit.cloudera.org:8080/#/c/10690/6/common/thrift/ImpalaService.thrift@96 PS6, Line 96: JITTER@ nit: JITTER@ Done -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 18 Jun 2018 20:53:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Hello Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10690 to look at the new patch set (#7). Change subject: IMPALA-7046: introduce "global" debug_actions .. IMPALA-7046: introduce "global" debug_actions The motivation is to add jitter to backend startup in test_failpoints. The race in IMPALA-7033 can be reproduced by adding jitter to the exec rpcs when some backends fail. Let's add jitter to test_failpoints to get better coverage of exec startup races. This builds on top of the debug action extensions added in the async admission control patch by allowing the new "global" debug actions (i.e. actions that can be used in points outside of the ExecNodes). See the code comments for details. For now, we're only using the SLEEP and JITTER commands, but I've included a FAIL command as well since I'll want to use that to write a test for IMPALA-6788 to simulate exec rpc failure. Note that I don't bother resolving the actions ahead of time (like we do for ExecNode actions). It doesn't seem worth it since the resolution only needs to occur after we've matched the label and I don't expect the same label to be hit many times within a single thread. We can always optimize this later if needed. Instead, just continue compiling it out of Release builds. Testing: - Verified that test_failpoints can reproduce the race in IMPALA-7033 by reverting that fix and testing. - Ran the modified tests and grepped the impalad log to see that the sleeps are still occuring. - Manually verify global FAIL command (in a build with another patch). Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 --- M be/src/runtime/coordinator.cc M be/src/runtime/debug-options.cc M be/src/scheduling/admission-controller.cc M be/src/service/client-request-state.cc M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_admission_controller.py M tests/failure/test_failpoints.py M tests/query_test/test_observability.py 10 files changed, 191 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/10690/7 -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 ) Change subject: IMPALA-7046: introduce "global" debug_actions .. Patch Set 6: (1 comment) This is a rebase but required fix up to remove the NDEBUG and do the empty() check like Tim added to the old code. http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/util/debug-util.h@129 PS6, Line 129: WARN_UNUSED_RESULT gcc complains if this is at the end for a function definition, for some reason. So I put it here. -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Sat, 16 Jun 2018 00:04:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7046: introduce "global" debug actions
Hello Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10690 to look at the new patch set (#6). Change subject: IMPALA-7046: introduce "global" debug_actions .. IMPALA-7046: introduce "global" debug_actions The motivation is to add jitter to backend startup in test_failpoints. The race in IMPALA-7033 can be reproduced by adding jitter to the exec rpcs when some backends fail. Let's add jitter to test_failpoints to get better coverage of exec startup races. This builds on top of the debug action extensions added in the async admission control patch by allowing the new "global" debug actions (i.e. actions that can be used in points outside of the ExecNodes). See the code comments for details. For now, we're only using the SLEEP and JITTER commands, but I've included a FAIL command as well since I'll want to use that to write a test for IMPALA-6788 to simulate exec rpc failure. Note that I don't bother resolving the actions ahead of time (like we do for ExecNode actions). It doesn't seem worth it since the resolution only needs to occur after we've matched the label and I don't expect the same label to be hit many times within a single thread. We can always optimize this later if needed. Instead, just continue compiling it out of Release builds. Testing: - Verified that test_failpoints can reproduce the race in IMPALA-7033 by reverting that fix and testing. - Ran the modified tests and grepped the impalad log to see that the sleeps are still occuring. - Manually verify global FAIL command (in a build with another patch). Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 --- M be/src/runtime/coordinator.cc M be/src/runtime/debug-options.cc M be/src/scheduling/admission-controller.cc M be/src/service/client-request-state.cc M be/src/util/debug-util.cc M be/src/util/debug-util.h M common/thrift/ImpalaService.thrift M tests/custom_cluster/test_admission_controller.py M tests/failure/test_failpoints.py M tests/query_test/test_observability.py 10 files changed, 194 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/10690/6 -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-110 (part 2): Refactor PartitionedAggregationNode
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 ) Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode .. Patch Set 2: > (14 comments) > > While developing this patch, I maintained a branch with a series of > commits separating the work out into several mostly mechanical > steps to keep everything straight. > Does each step function on it's own? it might be easier to just review and commit each step individually (or at least some grouping of the steps) so that big mechanical changes are easier to review. -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 15 Jun 2018 23:56:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7179: allow multiple scratch dirs per device=true by default
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10736 ) Change subject: IMPALA-7179: allow_multiple_scratch_dirs_per_device=true by default .. Patch Set 2: ok -- To view, visit http://gerrit.cloudera.org:8080/10736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23394c9949ae4cd0a21d7bb25551371b3198e76c Gerrit-Change-Number: 10736 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 15 Jun 2018 23:47:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h File be/src/runtime/reservation-manager.h: http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@33 PS2, Line 33: which with? http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@37 PS2, Line 37: static void Register(ReservationManager* rm, std::string name, please document that http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@39 PS2, Line 39: TBackendResourceProfile resource_profile, const TDebugOptions you probably meant to use references here. http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.cc File be/src/runtime/reservation-manager.cc: http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.cc@31 PS2, Line 31: Register this looks more like an Init() function (and doesn't have to be static given the first parameters is effectively 'this'). -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 15 Jun 2018 23:47:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7179: allow multiple scratch dirs per device=true by default
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10736 ) Change subject: IMPALA-7179: allow_multiple_scratch_dirs_per_device=true by default .. Patch Set 2: Code-Review+2 Seems better. Is there any weirdness/surprise that can happen with this change in default that we should document? -- To view, visit http://gerrit.cloudera.org:8080/10736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23394c9949ae4cd0a21d7bb25551371b3198e76c Gerrit-Change-Number: 10736 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 15 Jun 2018 23:38:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6812: Fix flaky Kudu scan tests
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10503 ) Change subject: IMPALA-6812: Fix flaky Kudu scan tests .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10503/4/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/10503/4/be/src/service/query-options.h@140 PS4, Line 140: ADVANCED so this will be documented and expected to work in some reasonable way? if not, maybe we should make it DEVELOPMENT. -- To view, visit http://gerrit.cloudera.org:8080/10503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70df84f2cbc663107f2ad029565d3c15bdfbd47c Gerrit-Change-Number: 10503 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 15 Jun 2018 20:22:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7174: fix test cancellation for RELEASE builds
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10722 ) Change subject: IMPALA-7174: fix test_cancellation for RELEASE builds .. Patch Set 2: Code-Review+2 Sounds fine. And I'll be sure not to revert it with https://gerrit.cloudera.org/#/c/10690/ -- To view, visit http://gerrit.cloudera.org:8080/10722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41da7b5ac58a468a8ed11969906f63df6d4b Gerrit-Change-Number: 10722 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 14 Jun 2018 23:45:37 + Gerrit-HasComments: No
