You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2016/08/10 20:49:09 UTC
[Impala-ASF-CR] IMPALA-3857: Add missing synchronization in KuduScanNode
Matthew Jacobs has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3888
Change subject: IMPALA-3857: Add missing synchronization in KuduScanNode
......................................................................
IMPALA-3857: Add missing synchronization in KuduScanNode
Attempt to fix IMPALA-3953 which isn't reproducing locally
or consistently, but seems to fail occasionally on jenkins.
After a previous fix for IMPALA-3857, KuduScanNodeTest
TestLimitsAreEnforced (BE test) seems to occasionally race
shutting down after a scan with a limit, resulting in a
scanner thread taking a lock_ that isn't valid and crashing
the process. While I cannot reproduce the issue or a clear
cut culprit, my theory is that the change for IMPALA-3857
changed the timing of the shutdown and exposed an existing
race.
This change is an attempt to fix a race around the limit and
shutdown paths where synchronization was missing. I'm not
completely sure this is the fix, but it is a bug worth
fixing regardless.
Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
---
M be/src/exec/kudu-scan-node.cc
1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/3888/1
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3953: Add missing synchronization in KuduScanNode
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3888
to look at the new patch set (#2).
Change subject: IMPALA-3953: Add missing synchronization in KuduScanNode
......................................................................
IMPALA-3953: Add missing synchronization in KuduScanNode
Attempt to fix IMPALA-3953 which isn't reproducing locally
or consistently, but seems to fail occasionally on jenkins.
After a previous fix for IMPALA-3857, KuduScanNodeTest
TestLimitsAreEnforced (BE test) seems to occasionally race
shutting down after a scan with a limit, resulting in a
scanner thread taking a lock_ that isn't valid and crashing
the process. While I cannot reproduce the issue or a clear
cut culprit, my theory is that the change for IMPALA-3857
changed the timing of the shutdown and exposed an existing
race.
This change is an attempt to fix a race around the limit and
shutdown paths where synchronization was missing. I'm not
completely sure this is the fix, but it is a bug worth
fixing regardless.
Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
---
M be/src/exec/kudu-scan-node.cc
1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/3888/2
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3857: Add missing synchronization in KuduScanNode
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3857: Add missing synchronization in KuduScanNode
......................................................................
Patch Set 1: Code-Review+1
I don't think that this fixes IMPALA-3953, but it seems like it's worth doing anyway.
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Fixes for KuduScanNode BE test failure
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3953: Fixes for KuduScanNode BE test failure
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3857: Add missing synchronization in KuduScanNode
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3857: Add missing synchronization in KuduScanNode
......................................................................
Patch Set 1:
I'm not sure that I understand the scenario. I think this is worth fixing, but it seems like calling scanner_threads_.JoinAll() would prevent the scanner threads from living beyond the point when the exec node and queue were valid.
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Add missing synchronization in KuduScanNode
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3953: Add missing synchronization in KuduScanNode
......................................................................
Patch Set 2:
> I don't think that this fixes IMPALA-3953, but it seems like it's
> worth doing anyway.
As we discussed in person, there's a bit more context wasn't yet in the JIRA: I eventually was able to hack a jenkins job to get it to fail and then quickly gather the stack on the machine myself. The crash stack shows a ScannerThread calling into the ThreadResourceMgr to release the token, which then calls back into KuduScanNode's ThreadAvailableCb, and that's where it crashes trying to take the lock_. That leads me to believe the KuduScanNode was torn down improperly. I can't figure out how that could have happened (a few other things that look suspicious I've reasoned through), and this is the only related issue that affects the crash scenario. I agree it doesn't seem too promising, but worth a shot and I need to step away from this bug for some time.
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Fixes for KuduScanNode BE test failure
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3953: Fixes for KuduScanNode BE test failure
......................................................................
IMPALA-3953: Fixes for KuduScanNode BE test failure
After a previous fix for IMPALA-3857, KuduScanNodeTest
TestLimitsAreEnforced (BE test) occasionally throws when a
scanner thread takes a lock_ that isn't valid, crashing the
process.
It looks like the issue is likely that TestScanEmptyString
isn't closing its KuduScanNode, and a lingering
ScannerThread may end up touching invalid memory later.
This fixes the test case and also:
1) Adds some missing synchronization in KuduScanNode which
was found in the process of investigating this issue.
2) Adds a DCHECK on ~KuduScanNode() to ensure it was closed.
This was tested by running KuduScanNodeTest in a loop for 5
hours. Without the fix, the failure was produced within
several hours.
Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Reviewed-on: http://gerrit.cloudera.org:8080/3888
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/kudu-scan-node-test.cc
M be/src/exec/kudu-scan-node.cc
2 files changed, 4 insertions(+), 0 deletions(-)
Approvals:
Internal Jenkins: Verified
Dan Hecht: Looks good to me, approved
Tim Armstrong: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3857: Add missing synchronization in KuduScanNode
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3857: Add missing synchronization in KuduScanNode
......................................................................
Patch Set 1:
> I'm not sure that I understand the scenario. I think this is worth
> fixing, but it seems like calling scanner_threads_.JoinAll() would
> prevent the scanner threads from living beyond the point when the
> exec node and queue were valid.
Close() does call scanner_threads_.JoinAll() already. I'd been trying to figure out how a ScannerThread could live past the call to Close() but haven't been able to find anything that really explains it.
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Add missing synchronization in KuduScanNode
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3953: Add missing synchronization in KuduScanNode
......................................................................
Patch Set 2:
Would it make sense to add a d'tor to KuduScanNode that just DCHECK(is_closed_) (or maybe even to ~ExecNode)? If there's a path where Close() is not called on an exec node that's a bad bug, per the comments on Close().
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Fixes for KuduScanNode BE test failure
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3953: Fixes for KuduScanNode BE test failure
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Fixes for KuduScanNode BE test failure
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3953: Fixes for KuduScanNode BE test failure
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3953: Fixes for KuduScanNode BE test failure
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3888
to look at the new patch set (#3).
Change subject: IMPALA-3953: Fixes for KuduScanNode BE test failure
......................................................................
IMPALA-3953: Fixes for KuduScanNode BE test failure
After a previous fix for IMPALA-3857, KuduScanNodeTest
TestLimitsAreEnforced (BE test) occasionally throws when a
scanner thread takes a lock_ that isn't valid, crashing the
process.
It looks like the issue is likely that TestScanEmptyString
isn't closing its KuduScanNode, and a lingering
ScannerThread may end up touching invalid memory later.
This fixes the test case and also:
1) Adds some missing synchronization in KuduScanNode which
was found in the process of investigating this issue.
2) Adds a DCHECK on ~KuduScanNode() to ensure it was closed.
This was tested by running KuduScanNodeTest in a loop for 5
hours. Without the fix, the failure was produced within
several hours.
Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
---
M be/src/exec/kudu-scan-node-test.cc
M be/src/exec/kudu-scan-node.cc
2 files changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/3888/3
--
To view, visit http://gerrit.cloudera.org:8080/3888
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16be206c60a692d2a26d719de8cc45e859b06e97
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>