[SPARK-44993][CORE] Add ShuffleChecksumUtils.compareChecksums by reusing ShuffleChecksumTestHelp.compareChecksums#42707
Conversation
…pe to ShuffleChecksumUtils
dongjoon-hyun
changed the title
compareChecksums from ShuffleChecksumTestHelpe to ShuffleChecksumUtilscompareChecksums from ShuffleChecksumTestHelp to ShuffleChecksumUtils
dongjoon-hyun
changed the title
compareChecksums from ShuffleChecksumTestHelp to ShuffleChecksumUtilsShuffleChecksumUtils.compareChecksums by reusing ShuffleChecksumTestHelp..compareChecksums
dongjoon-hyun
changed the title
ShuffleChecksumUtils.compareChecksums by reusing ShuffleChecksumTestHelp..compareChecksumsShuffleChecksumUtils.compareChecksums by reusing ShuffleChecksumTestHelp.compareChecksums
|
|
| /** | ||
| * Ensure that the checksum values are consistent with index file and data file. | ||
| */ | ||
| def compareChecksums( |
There was a problem hiding this comment.
Should we put this into ShuffleChecksumHelper?
There was a problem hiding this comment.
ShuffleChecksumHelper is test code helper under test/scala/.... So we can not reuse it in the main code.
core/src/test/scala/org/apache/spark/shuffle/ShuffleChecksumTestHelper.scala
There was a problem hiding this comment.
Hmm, I meant ShuffleChecksumHelper not ShuffleChecksumTestHelper.
There was a problem hiding this comment.
Oh, I misread that.
BTW, I also considered that but I decided not to do because it's Java class which means we need to re-implement the whole logic. I guess you also suggested a moving from Scala to Scala code, not Scala to Java. Or, did you suggest to reimplement with Java?
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/checksum/ShuffleChecksumHelper.java
There was a problem hiding this comment.
Ah I see. No, I didn't realized that ShuffleChecksumHelper is Java code. Thanks for reply.
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
Looks good to me. Just wondering if we should put it into ShuffleChecksumHelper directly.
|
Thank you for review. I replied #42707 (comment) . |
|
Merged to master for Apache Spark 4. |
…using `ShuffleChecksumTestHelp.compareChecksums` ### What changes were proposed in this pull request? This PR aims to add `ShuffleChecksumUtils.compareChecksums` by reusing the existing test code `ShuffleChecksumTestHelp.compareChecksums` in order to reuse the functionality in the main code. ### Why are the changes needed? This is very useful in the test code. We can take advantage of this verification logic in `core` module. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs with the existing test codes because this is a kind of refactoring. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#42707 from dongjoon-hyun/SPARK-44993. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 5db58f9) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
2 participants
What changes were proposed in this pull request?
This PR aims to add
ShuffleChecksumUtils.compareChecksumsby reusing the existing test codeShuffleChecksumTestHelp.compareChecksumsin order to reuse the functionality in the main code.Why are the changes needed?
This is very useful in the test code. We can take advantage of this verification logic in
coremodule.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass the CIs with the existing test codes because this is a kind of refactoring.
Was this patch authored or co-authored using generative AI tooling?
No.