Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-33916][CORE] Fix fallback storage offset and improve compression codec test coverage #30934

Closed
wants to merge 2 commits into from

Conversation

dongjoon-hyun
Copy link
Member

What changes were proposed in this pull request?

This PR aims to fix offset bug and improve compression codec test coverage.

Why are the changes needed?

When the user choose a non-default codec, it causes a failure.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the extended test suite.

@github-actions github-actions bot added the CORE label Dec 26, 2020
val rdd2 = rdd1.map(x => (x % 2, 1))
val rdd3 = rdd2.reduceByKey(_ + _)
assert(rdd3.collect() === Array((0, 5), (1, 5)))
Seq("lz4", "snappy", "zstd").foreach { codec =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc of IO_COMPRESSION_CODEC says that it supports lz4, lzf, snappy, and zstd. Should we test lzf too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll add that, @MaxGekk .

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the user choose a non-default codec, it causes a failure.

Hm, the bug is caused by wrong offset calculation or non-default codec?

@SparkQA
Copy link

SparkQA commented Dec 26, 2020

Test build #133395 has finished for PR 30934 at commit 2c54d10.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

The lz4 (the default) compression codec seems to be robust on it while the other compression code is sensitive in some cases, @viirya .

Hm, the bug is caused by wrong offset calculation or non-default codec?

@@ -158,7 +158,7 @@ object FallbackStorage extends Logging {
val name = ShuffleDataBlockId(shuffleId, mapId, NOOP_REDUCE_ID).name
val dataFile = new Path(fallbackPath, s"$appId/$shuffleId/$name")
val f = fallbackFileSystem.open(dataFile)
val size = nextOffset - 1 - offset
val size = nextOffset - offset
logDebug(s"To byte array $size")
Copy link
Contributor

@mridulm mridulm Dec 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this bug, I am wondering if we want to refactor IndexShuffleBlockResolver.read such that we can reuse it here as well ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, for Apache Spark 3.2.0, @mridulm ? Currently, this PR is aiming to provide this fix for Apache Spark 3.1.0 RC before next Monday.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks @dongjoon-hyun !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @mridulm !

@SparkQA
Copy link

SparkQA commented Dec 27, 2020

Test build #133400 has finished for PR 30934 at commit 1550b55.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change LGTM

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider refactoring later.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Dec 28, 2020

Thank you for review and approval, @HyukjinKwon .

Please let me know if you have other comments, @MaxGekk and @mridulm .

@dongjoon-hyun
Copy link
Member Author

Thank you all!
Merged to master/3.1.

dongjoon-hyun added a commit that referenced this pull request Dec 29, 2020
…on codec test coverage

### What changes were proposed in this pull request?

This PR aims to fix offset bug and improve compression codec test coverage.

### Why are the changes needed?

When the user choose a non-default codec, it causes a failure.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the extended test suite.

Closes #30934 from dongjoon-hyun/SPARK-33916.

Authored-by: Dongjoon Hyun <dhyun@apple.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 6497ccb)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
@dongjoon-hyun dongjoon-hyun deleted the SPARK-33916 branch December 29, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants