You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sahil Takiar (Code Review)" <ge...@cloudera.org> on 2019/09/11 17:02:04 UTC

[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 <st...@cloudera.com>

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Sep 2019 01:31:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 02:03:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 19:11:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 22:30:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 18:34:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 22:30:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 22:27:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 02:45:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Sep 2019 17:43:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Sep 2019 21:42:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Sep 2019 00:53:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 14:51:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 19:03:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Sep 2019 05:35:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2019 14:51:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
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 <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>