You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2018/07/31 23:31:37 UTC
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11096
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
While Prepare()-ing a fragment instance, if we fail to initialize the
runtime filter bank, we will exit FIS::Prepare() without acquiring a
thread token (AcquireThreadToken()):
FIS::Finalize() is called always regardless of whether the fragment
instance succeeded or failed. And FIS::Finalize() tries to ReleaseThreadToken()
even though it might not have gotten acquired, causing a DCHECK to be hit.
This patch fixes it by making sure that no failable code is run before
acquiring the thread token, thereby ensuring that the thread token is
always acquired and thus avoiding the above crash.
A test is added to confirm this as well. This test crashes without the
code changes in this patch.
Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
---
M be/src/runtime/fragment-instance-state.cc
M tests/failure/test_failpoints.py
2 files changed, 16 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/11096/1
--
To view, visit http://gerrit.cloudera.org:8080/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11096 )
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Carry +2.
http://gerrit.cloudera.org:8080/#/c/11096/2/tests/failure/test_failpoints.py
File tests/failure/test_failpoints.py:
http://gerrit.cloudera.org:8080/#/c/11096/2/tests/failure/test_failpoints.py@147
PS2, Line 147: query_options={'debug_action':debug_action})
> Should we also verify that the query actually failed ?
It is already doing that since the function called is:
execute_query_expect_failure()
--
To view, visit http://gerrit.cloudera.org:8080/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 23:01:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11096 )
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Aug 2018 02:20:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11096 )
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11096/2/tests/failure/test_failpoints.py
File tests/failure/test_failpoints.py:
http://gerrit.cloudera.org:8080/#/c/11096/2/tests/failure/test_failpoints.py@147
PS2, Line 147: query_options={'debug_action':debug_action})
Should we also verify that the query actually failed ?
--
To view, visit http://gerrit.cloudera.org:8080/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 22:46:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11096 )
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/122/ : 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/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 00:03:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11096 )
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2900/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 23:01:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
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/11096 )
Change subject: IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
......................................................................
IMPALA-7376: DCHECK hit if a fragment instance fails to initialize the filter bank
While Prepare()-ing a fragment instance, if we fail to initialize the
runtime filter bank, we will exit FIS::Prepare() without acquiring a
thread token (AcquireThreadToken()):
FIS::Finalize() is called always regardless of whether the fragment
instance succeeded or failed. And FIS::Finalize() tries to ReleaseThreadToken()
even though it might not have gotten acquired, causing a DCHECK to be hit.
This patch fixes it by making sure that no failable code is run before
acquiring the thread token, thereby ensuring that the thread token is
always acquired and thus avoiding the above crash.
A test is added to confirm this as well. This test crashes without the
code changes in this patch.
Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Reviewed-on: http://gerrit.cloudera.org:8080/11096
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/fragment-instance-state.cc
M tests/failure/test_failpoints.py
2 files changed, 16 insertions(+), 2 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1d6e7afc18fe2f0e1e29d2bd8a5f804a78f7043a
Gerrit-Change-Number: 11096
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>