[SPARK-31511][SQL][2.4] Make BytesToBytesMap iterators thread-safe #29605

cxzl25 · 2020-09-01T04:18:12Z

What changes were proposed in this pull request?

This PR increases the thread safety of the BytesToBytesMap:

It makes the iterator() and destructiveIterator() methods used their own Location object. This used to be shared, and this was causing issues when the map was being iterated over in two threads by two different iterators.
Removes the safeIterator() function. This is not needed anymore.
Improves the documentation of a couple of methods w.r.t. thread-safety.

Why are the changes needed?

It is unexpected an iterator shares the object it is returning with all other iterators. This is a violation of the iterator contract, and it causes issues with iterators over a map that are consumed in different threads.

Does this PR introduce any user-facing change?

No

How was this patch tested?

add ut

### What changes were proposed in this pull request? This PR increases the thread safety of the `BytesToBytesMap`: - It makes the `iterator()` and `destructiveIterator()` methods used their own `Location` object. This used to be shared, and this was causing issues when the map was being iterated over in two threads by two different iterators. - Removes the `safeIterator()` function. This is not needed anymore. - Improves the documentation of a couple of methods w.r.t. thread-safety. ### Why are the changes needed? It is unexpected an iterator shares the object it is returning with all other iterators. This is a violation of the iterator contract, and it causes issues with iterators over a map that are consumed in different threads. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing tests. Closes apache#28286 from hvanhovell/SPARK-31511. Authored-by: herman <herman@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit cf60384)

dongjoon-hyun · 2020-09-01T16:10:49Z

ok to test

dongjoon-hyun · 2020-09-01T16:11:06Z

Thank you so much, @cxzl25 .

cc @hvanhovell and @cloud-fan

dongjoon-hyun · 2020-09-01T16:12:10Z

Also, cc @ScrapCodes for Apache Spark 2.4.7 since this is a correctness fix.

SparkQA · 2020-09-01T16:13:48Z

Test build #128154 has finished for PR 29605 at commit 6492259.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan

LGTM if tests pass

ScrapCodes · 2020-09-02T06:16:25Z

Please correct, if I am wrong, currently there are no blockers for 2.4.7 release, but we are still waiting for this one to be merged.

SparkQA · 2020-09-02T07:05:02Z

Test build #128180 has finished for PR 29605 at commit 49b4925.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-09-02T08:06:41Z

I don't think it's a blocker. But if the RC is not started, it's probably better to wait a bit for this patch.

SparkQA · 2020-09-02T08:11:46Z

Test build #128199 has finished for PR 29605 at commit 5ed2f28.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-09-02T08:36:25Z

retest this please

SparkQA · 2020-09-02T08:41:28Z

Test build #128200 has finished for PR 29605 at commit 5ed2f28.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-09-02T08:45:41Z

jenkins is unstable now, let's wait for the github action results.

cxzl25 · 2020-09-02T14:08:20Z

I saw the last backport to the pr of 2.4, and the log of the github action result also reported a similar error pyspark ArrowTest.
https://github.com/apache/spark/runs/1044010610

The java arrow version of the master branch is 0.15.1, and the version of the 2.4 branch is 0.10.0.

Build_and_test.yml did not specify the pyarrow version when pip was installed, so the version that may be installed is the latest 1.0.1 version.

I am not sure if the incompatibility between 0.10 and 1.0.1 caused the check to fail.

I see that the pyspark test on jenkins is passed.

I try to submit and trigger another jenkins test.

cloud-fan · 2020-09-02T16:20:45Z

retest this please

dongjoon-hyun · 2020-09-02T18:21:16Z

It's really weird because both Jenkins and GitHub Action are flaky irrelevantly.

dongjoon-hyun · 2020-09-02T18:21:23Z

Retest this please.

SparkQA · 2020-09-02T21:30:42Z

Test build #128210 has finished for PR 29605 at commit 30eed99.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-03T03:30:57Z

Test build #128224 has finished for PR 29605 at commit cc8d277.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-03T05:20:22Z

Test build #128227 has finished for PR 29605 at commit 5fbcfaf.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-03T06:52:47Z

Test build #128232 has finished for PR 29605 at commit 9e689c5.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-04T14:34:57Z

Test build #128301 has finished for PR 29605 at commit 2c3ff67.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-04T17:25:10Z

Test build #128310 has finished for PR 29605 at commit d4c785d.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-05T07:05:01Z

Test build #128317 has finished for PR 29605 at commit 0e39f7a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cxzl25 · 2020-09-07T03:51:51Z

I removed the code I changed in the commit (0e39f7a), but the jenkins and github action tests still don't pass, which is very strange.

cloud-fan · 2020-09-07T05:17:41Z

retest this please

cloud-fan · 2020-09-07T05:18:03Z

BTW, @cxzl25 you can use empty git commit to trigger test

SparkQA · 2020-09-07T07:05:02Z

Test build #128336 has finished for PR 29605 at commit 0e39f7a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-09-07T07:05:02Z

Test build #128337 has finished for PR 29605 at commit a508102.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cxzl25 · 2020-09-07T07:54:56Z

Empty commits don't trigger a github action(No test results found!), but the jenkins test is a failure.

cloud-fan · 2020-09-07T12:27:25Z

retest this please

SparkQA · 2020-09-07T12:43:36Z

Test build #128355 has finished for PR 29605 at commit a508102.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-09-07T16:13:00Z

@cxzl25 why does this patch become empty? You can add empty commits to trigger tests, but you can't turn the patch into empty...

cxzl25 · 2020-09-07T16:28:02Z

@cloud-fan
I just want to verify whether the test of the current branch works normally, the test still seems to fail.
Now i resubmit the code.

SparkQA · 2020-09-07T18:56:36Z

Test build #128360 has finished for PR 29605 at commit 37465c5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class First(child: Expression, ignoreNulls: Boolean)
case class Last(child: Expression, ignoreNulls: Boolean)

cloud-fan · 2020-09-08T03:23:56Z

jenkins is happy finally ...

Thanks, merging to 2.4!

### What changes were proposed in this pull request? This PR increases the thread safety of the `BytesToBytesMap`: - It makes the `iterator()` and `destructiveIterator()` methods used their own `Location` object. This used to be shared, and this was causing issues when the map was being iterated over in two threads by two different iterators. - Removes the `safeIterator()` function. This is not needed anymore. - Improves the documentation of a couple of methods w.r.t. thread-safety. ### Why are the changes needed? It is unexpected an iterator shares the object it is returning with all other iterators. This is a violation of the iterator contract, and it causes issues with iterators over a map that are consumed in different threads. ### Does this PR introduce any user-facing change? No ### How was this patch tested? add ut Closes #29605 from cxzl25/SPARK-31511. Lead-authored-by: sychen <sychen@ctrip.com> Co-authored-by: herman <herman@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2020-09-08T03:24:34Z

sql/core/src/test/scala/org/apache/spark/sql/execution/joins/HashedRelationSuite.scala

@@ -341,6 +341,45 @@ class HashedRelationSuite extends SparkFunSuite with SharedSQLContext {
    assert(java.util.Arrays.equals(os.toByteArray, os2.toByteArray))
  }

+  test("SPARK-31511: Make BytesToBytesMap iterators thread-safe") {


can you open a new PR to add this test for master/3.0?

ok, I submit a pull request to add test.
#29669

dongjoon-hyun · 2020-09-08T03:39:43Z

Thank you, @cxzl25 and @cloud-fan .

ScrapCodes · 2020-09-08T04:36:45Z

Now, I can go ahead and tag the release. Good work. ( A long battle with CI)

dongjoon-hyun · 2020-09-08T06:22:22Z

Thank you for the release preparation, @ScrapCodes .

hvanhovell and others added 2 commits September 1, 2020 12:12

add ut

6492259

probot-autolabeler bot added CORE SQL labels Sep 1, 2020

cloud-fan approved these changes Sep 1, 2020

View reviewed changes

trigger test

49b4925

trigger test v2

5ed2f28

Merge branch 'branch-2.4' into SPARK-31511

37465c5

cxzl25 force-pushed the SPARK-31511 branch from a508102 to 37465c5 Compare September 7, 2020 16:23

cloud-fan reviewed Sep 8, 2020

View reviewed changes

cloud-fan closed this Sep 8, 2020

[SPARK-31511][SQL][2.4] Make BytesToBytesMap iterators thread-safe #29605

[SPARK-31511][SQL][2.4] Make BytesToBytesMap iterators thread-safe #29605

Conversation

cxzl25 commented Sep 1, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

dongjoon-hyun commented Sep 1, 2020

dongjoon-hyun commented Sep 1, 2020 • edited Loading

dongjoon-hyun commented Sep 1, 2020

SparkQA commented Sep 1, 2020

cloud-fan left a comment

Choose a reason for hiding this comment

ScrapCodes commented Sep 2, 2020

SparkQA commented Sep 2, 2020

cloud-fan commented Sep 2, 2020

SparkQA commented Sep 2, 2020

cloud-fan commented Sep 2, 2020

SparkQA commented Sep 2, 2020

cloud-fan commented Sep 2, 2020

cxzl25 commented Sep 2, 2020

cloud-fan commented Sep 2, 2020

dongjoon-hyun commented Sep 2, 2020

dongjoon-hyun commented Sep 2, 2020

SparkQA commented Sep 2, 2020

SparkQA commented Sep 3, 2020

SparkQA commented Sep 3, 2020

SparkQA commented Sep 3, 2020

SparkQA commented Sep 4, 2020

SparkQA commented Sep 4, 2020

SparkQA commented Sep 5, 2020

cxzl25 commented Sep 7, 2020

cloud-fan commented Sep 7, 2020

cloud-fan commented Sep 7, 2020

SparkQA commented Sep 7, 2020

SparkQA commented Sep 7, 2020

cxzl25 commented Sep 7, 2020

cloud-fan commented Sep 7, 2020

SparkQA commented Sep 7, 2020

cloud-fan commented Sep 7, 2020

cxzl25 commented Sep 7, 2020

SparkQA commented Sep 7, 2020

cloud-fan commented Sep 8, 2020

cloud-fan Sep 8, 2020

Choose a reason for hiding this comment

cxzl25 Sep 8, 2020

Choose a reason for hiding this comment

dongjoon-hyun commented Sep 8, 2020

ScrapCodes commented Sep 8, 2020

dongjoon-hyun commented Sep 8, 2020

dongjoon-hyun commented Sep 1, 2020 •

edited

Loading