[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 19 Sep 2019 19:03:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs Adds several "failpoint" tests to test_result_spooling.py. These tests use debug_actions spread throughout buffered-plan-root-sink.cc to trigger failures while result spooling is running. The tests validate that all queries gracefully fail and do not cause any impalad crashes. Fixed a few bugs that came up when adding these tests, as well as the crash reported in IMPALA-8924 (which is now covered by the failpoint tests added in this patch). The first bug fixed was a DCHECK in SpillableRowBatchQueue::IsEmpty() where the method was being called after the queue had been closed. The fix is to only call IsEmpty() if IsOpen() returns true. The second bug was an issue in the cancellation path where BufferedPlanRootSink::GetNext would enter an infinite loop if the query was cancelled and then GetNext was called. The fix is to check the cancellation state in the outer while loop. Testing: * Added new tests to test_result_spooling.py * Ran core tests Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Reviewed-on: http://gerrit.cloudera.org:8080/14214 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/exec/buffered-plan-root-sink.cc M be/src/exec/buffered-plan-root-sink.h M tests/common/impala_test_suite.py M tests/query_test/test_result_spooling.py A tests/util/failpoints_util.py 5 files changed, 137 insertions(+), 14 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4965/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 19 Sep 2019 14:51:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 19 Sep 2019 14:51:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 4: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4961/ -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Thu, 19 Sep 2019 02:45:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4961/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 22:30:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 22:30:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 22:27:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4587/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 19:11:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Sahil Takiar has posted comments on this change. (
http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix
DCHECKs
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14214/2/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/14214/2/be/src/exec/buffered-plan-root-sink.h@147
PS2, Line 147: bool IsQueueEmpty(RuntimeState* state) const {
> It would be nice if we could add the following DCHECK:
Done
http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py@404
PS2, Line 404: ecute_query_expect_debug_
> or assert False, "Expected Failure"
yeah, this pattern was actually copied from test_failpoints.py - so looks like
that has the same bug as well - filed IMPALA-8952 to fix this
had to do some re-factoring of this class as well, a few test failures popped
up when changed it to `assert False, "Expected Failure".
I moved the DEBUG_ACTION 5:GETNEXT:MEM_LIMIT_EXCEEDED|0:GETNEXT:DELAY and the
query select 1 from functional.alltypessmall a join functional.alltypessmall b
on a.id = b.id to a separate test case, mainly because the DEBUG_ACTION is
specific to the query (e.g. it injects a failure into the 5th node, and `select
* from functional.alltypes` does not even have 5 nodes).
--
To view, visit http://gerrit.cloudera.org:8080/14214
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932
Gerrit-Change-Number: 14214
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Wed, 18 Sep 2019 18:34:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14214 to look at the new patch set (#3). Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs Adds several "failpoint" tests to test_result_spooling.py. These tests use debug_actions spread throughout buffered-plan-root-sink.cc to trigger failures while result spooling is running. The tests validate that all queries gracefully fail and do not cause any impalad crashes. Fixed a few bugs that came up when adding these tests, as well as the crash reported in IMPALA-8924 (which is now covered by the failpoint tests added in this patch). The first bug fixed was a DCHECK in SpillableRowBatchQueue::IsEmpty() where the method was being called after the queue had been closed. The fix is to only call IsEmpty() if IsOpen() returns true. The second bug was an issue in the cancellation path where BufferedPlanRootSink::GetNext would enter an infinite loop if the query was cancelled and then GetNext was called. The fix is to check the cancellation state in the outer while loop. Testing: * Added new tests to test_result_spooling.py * Ran core tests Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 --- M be/src/exec/buffered-plan-root-sink.cc M be/src/exec/buffered-plan-root-sink.h M tests/common/impala_test_suite.py M tests/query_test/test_result_spooling.py A tests/util/failpoints_util.py 5 files changed, 137 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14214/3 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py File tests/query_test/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py@404 PS2, Line 404: assert 'Expected Failure' > Did you mean to assert 'Expected Failure' in sth.. ? or assert False, "Expected Failure" -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Wed, 18 Sep 2019 05:35:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix
DCHECKs
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14214/2/be/src/exec/buffered-plan-root-sink.h
File be/src/exec/buffered-plan-root-sink.h:
http://gerrit.cloudera.org:8080/#/c/14214/2/be/src/exec/buffered-plan-root-sink.h@147
PS2, Line 147: bool IsQueueEmpty() const {
It would be nice if we could add the following DCHECK:
DCHECK(!IsCancelledOrClosed(state));
http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/14214/2/tests/query_test/test_result_spooling.py@404
PS2, Line 404: assert 'Expected Failure'
Did you mean to assert 'Expected Failure' in sth.. ?
--
To view, visit http://gerrit.cloudera.org:8080/14214
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932
Gerrit-Change-Number: 14214
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Wed, 18 Sep 2019 02:03:04 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4560/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Sat, 14 Sep 2019 01:31:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Sahil Takiar has posted comments on this change. (
http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix
DCHECKs
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc@175
PS1, Line 175: while (IsQueueClosedOrEmpty() && sender_state_ ==
SenderState::ROWS_PENDING
: && !state->is_cancelled() && !timed_out) {
> As discussed offline, the contract seems simpler if we make it an invariant
It actually has to check if the query has been cancelled or if the sink has
been closed (because the sink can be closed before is_cancelled() is set to
true). I changed IsQueueClosedOrEmpty() back to IsQueueEmpty(), so now all the
loop have to check if the query has been cancelled or if the sink has been
closed before checking if the queue is empty. Since that check has to be
repeated in multiple places, I moved it into a method called
IsCancelledOrClosed. I'm not sure if thats much better than
IsQueueClosedOrEmpty - the reason I'm adding the helper method is to try and
simply the already complex condition in the while loops.
--
To view, visit http://gerrit.cloudera.org:8080/14214
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932
Gerrit-Change-Number: 14214
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sahil Takiar
Gerrit-Comment-Date: Sat, 14 Sep 2019 00:53:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14214 to look at the new patch set (#2). Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs Adds several "failpoint" tests to test_result_spooling.py. These tests use debug_actions spread throughout buffered-plan-root-sink.cc to trigger failures while result spooling is running. The tests validate that all queries gracefully fail and do not cause any impalad crashes. Fixed a few bugs that came up when adding these tests, as well as the crash reported in IMPALA-8924 (which is now covered by the failpoint tests added in this patch). The first bug fixed was a DCHECK in SpillableRowBatchQueue::IsEmpty() where the method was being called after the queue had been closed. The fix is to only call IsEmpty() if IsOpen() returns true. The second bug was an issue in the cancellation path where BufferedPlanRootSink::GetNext would enter an infinite loop if the query was cancelled and then GetNext was called. The fix is to check the cancellation state in the outer while loop. Testing: * Added new tests to test_result_spooling.py * Ran core tests Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 --- M be/src/exec/buffered-plan-root-sink.cc M be/src/exec/buffered-plan-root-sink.h M tests/query_test/test_result_spooling.py 3 files changed, 104 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14214/2 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix
DCHECKs
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc
File be/src/exec/buffered-plan-root-sink.cc:
http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc@175
PS1, Line 175: while (IsQueueClosedOrEmpty() && sender_state_ ==
SenderState::ROWS_PENDING
: && !state->is_cancelled() && !timed_out) {
As discussed offline, the contract seems simpler if we make it an invariant
that IsQueueEmpty() cannot be called after the plan root sink has been
cancelled.
IMHO, it seems a bit weird to still loop around if "the queue is closed" part
is true. It seems to be making assumption that state->is_cancelled() is called
afterwards. IMHO, the new interface IsQueueClosedOrEmpty() seems a tad error
prone as it returns true for two possible states and these two states may
potentially lead to drastically different actions (i.e. keep waiting vs
breaking out of the loop).
--
To view, visit http://gerrit.cloudera.org:8080/14214
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932
Gerrit-Change-Number: 14214
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Comment-Date: Fri, 13 Sep 2019 21:42:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 ) Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4542/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 11 Sep 2019 17:43:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs
Sahil Takiar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14214 Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs .. IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs Adds several "failpoint" tests to test_result_spooling.py. These tests use debug_actions spread throughout buffered-plan-root-sink.cc to trigger failures while result spooling is running. The tests validate that all queries gracefully fail and do not cause any impalad crashes. Fixed a few bugs that came up when adding these tests, as well as the crash reported in IMPALA-8924 (which is now covered by the failpoint tests added in this patch). The first bug fixed was a DCHECK in SpillableRowBatchQueue::IsEmpty() where the method was being called after the queue had been closed. The fix is to only call IsEmpty() if IsOpen() returns true. The second bug was an issue in the cancellation path where BufferedPlanRootSink::GetNext would enter an infinite loop if the query was cancelled and then GetNext was called. The fix is to check the cancellation state in the outer while loop. Testing: * Added new tests to test_result_spooling.py * Ran core tests Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 --- M be/src/exec/buffered-plan-root-sink.cc M be/src/exec/buffered-plan-root-sink.h M tests/query_test/test_result_spooling.py 3 files changed, 100 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/14214/1 -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar
