[SPARK-35263] [TEST] Refactor ShuffleBlockFetcherIteratorSuite to reduce duplicated code #32389

xkrogen · 2021-04-28T20:17:43Z

What changes were proposed in this pull request?

Introduce new shared methods to ShuffleBlockFetcherIteratorSuite to replace copy-pasted code. Use modern, Scala-like Mockito Answer syntax.

Why are the changes needed?

ShuffleFetcherBlockIteratorSuite has tons of duplicate code, like

spark/core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

Lines 172 to 185 in 0494dc9

    
           val iterator = new ShuffleBlockFetcherIterator( 
        
             taskContext, 
        
             transfer, 
        
             blockManager, 
        
             blocksByAddress, 
        
             (_, in) => in, 
        
             48 * 1024 * 1024, 
        
             Int.MaxValue, 
        
             Int.MaxValue, 
        
             Int.MaxValue, 
        
             true, 
        
             false, 
        
             metrics, 
        
             false)

. It's challenging to tell what the interesting parts are vs. what is just being set to some default/unused value.

Similarly but not as bad, there are many calls like the following

verify(transfer, times(1)).fetchBlocks(any(), any(), any(), any(), any(), any())
when(transfer.fetchBlocks(any(), any(), any(), any(), any(), any())).thenAnswer ...

These changes result in about 10% reduction in both lines and characters in the file:

# Before
> wc core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala
    1063    3950   43201 core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

# After
> wc core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala
     928    3609   39053 core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

It also helps readability, e.g.:

    val iterator = createShuffleBlockIteratorWithDefaults(
      transfer,
      blocksByAddress,
      maxBytesInFlight = 1000L
    )

Now I can clearly tell that maxBytesInFlight is the main parameter we're interested in here.

Does this PR introduce any user-facing change?

No, test only. There aren't even any behavior changes, just refactoring.

How was this patch tested?

Unit tests pass.

SparkQA · 2021-04-28T20:39:57Z

Test build #138054 has finished for PR 32389 at commit 322593f.

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

SparkQA · 2021-04-28T21:16:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42573/

SparkQA · 2021-04-28T21:16:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42573/

SparkQA · 2021-04-28T21:34:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42574/

SparkQA · 2021-04-28T21:34:40Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42574/

SparkQA · 2021-04-28T23:10:36Z

Test build #138055 has finished for PR 32389 at commit c96a16e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2021-04-29T00:48:55Z

cc @Ngone51 @mridulm FYI

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

Ngone51 · 2021-04-29T06:02:02Z

This's a great refactor. I left some minor comments. Overall, looks good to me.

xkrogen · 2021-04-29T16:23:29Z

Thanks @Ngone51 for the suggestions!

I put up two more commits. The first pretty directly answers your comments and has some other small fixes I noticed.

The second one is a bit larger and was inspired by your comment about moving the helper methods from the object to the class. All of the tests make use of val transfer = mock(classOf[BlockTransferService]) and then pass this around to the helper methods. I moved this to be a field instead and then we can remove transfer from being passed in the method signatures. I think it's cleaner but open to feedback on that part, I am happy to revert if you/others don't find it useful.

It does reduce the size by another ~40 lines:

> wc core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala
     894    3509   37951 core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

SparkQA · 2021-04-29T17:29:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42607/

SparkQA · 2021-04-29T17:33:49Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42607/

SparkQA · 2021-04-29T19:07:46Z

Test build #138087 has finished for PR 32389 at commit 596c3dd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

xkrogen · 2021-05-04T16:41:03Z

@Ngone51 do you have any more comments here? Thanks a lot for your comments so far!

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

Ngone51 · 2021-05-08T02:00:35Z

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

Maybe this could be extended to support the case of multiple blockmanagers, e.g.,

val blocksByAddress = Seq[(BlockManagerId, Seq[(BlockId, Long, Int)])]( (localBmId, localBlocks.keys.map(blockId => (blockId, 1L, 0)).toSeq), (remoteBmId, remoteBlocks.keys.map(blockId => (blockId, 1L, 1)).toSeq), (hostLocalBmId, hostLocalBlocks.keys.map(blockId => (blockId, 1L, 1)).toSeq) ).toIterator

We can pass in a Map[BlockManagerId, (blocks, size, mapIndex)] instead.

WDYT?

Good idea! I focused on the single-BM case because it was simpler, but you're right that there was still a lot of common logic to be reduced. Actually, your suggestion made me realize that we only ever use this method (and the new multi-BM method I created) to create an iterator which is then passed to getShuffleIteratorWithDefaults, so I just made getShuffleIteratorWithDefaults directly accept a Map. I think it's very clean now, thank you for the suggestion!

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

Ngone51 · 2021-05-08T02:05:38Z

cc @mridulm @tgravescs @attilapiros

xkrogen · 2021-05-10T16:22:31Z

Great comments @Ngone51 ! Pushed up a new set of commits addressing your comments.

SparkQA · 2021-05-10T17:07:19Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42860/

SparkQA · 2021-05-10T17:07:20Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42860/

SparkQA · 2021-05-10T18:56:01Z

Test build #138338 has finished for PR 32389 at commit 196cb06.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

otterc · 2021-05-11T15:29:19Z

@xkrogen @Ngone51 @mridulm Gentle ping folks to check if this PR can be merged. To me it looks like it is ready.
I need to rework the tests in my PR once this is merged.

.idea/vcs.xml

mridulm · 2021-05-14T21:47:12Z

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

nit: I was oscillating between this being a Map vs Seq ... currently, a Map is fine based on how ShuffleBlockFetcherIterator is used ... but might be something we revisit in future.

mridulm

Just some minor nits, really nice work @xkrogen !

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

SparkQA · 2021-05-17T19:43:42Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43161/

SparkQA · 2021-05-17T19:50:45Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43161/

SparkQA · 2021-05-17T21:15:36Z

Test build #138641 has finished for PR 32389 at commit 4fcc6be.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mridulm

Looks good to me, thanks for working on this @xkrogen !
+CC @Ngone51, @otterc PTAL

otterc

Looks good to me. Just a minor nit

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

…especially instantiating the ShuffleBlockFetcherIterator

…ito syntax and pull out common calls to when(transfer.fetchBlocks) and verify(transfer, ...).fetchBlocks

…BlocksCount to verifyFetchBlocksInvocationCount. Restore helpful comments. Remove one redundant parameter override. Fix upa few minor issues such as unnecessary specification of very long types.

…sing it around in method signatures.

…dress which accepts a Map

…tly accept a map of block info

…y tests

SparkQA · 2021-05-18T19:03:20Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43211/

SparkQA · 2021-05-18T19:37:37Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43211/

SparkQA · 2021-05-18T20:31:54Z

Test build #138690 has finished for PR 32389 at commit eea80f5.

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

Ngone51

LGTM!

mridulm · 2021-05-19T03:39:07Z

Merging to master, thanks @xkrogen.
Thanks for the reviews @Ngone51 , @otterc !

xkrogen · 2021-05-19T16:45:31Z

Thanks for the reviews @mridulm , @otterc, and @Ngone51 ! Much appreciated.

github-actions bot added the CORE label Apr 28, 2021

xkrogen force-pushed the xkrogen-spark-35263-refactor-shuffleblockfetcheriteratorsuite branch from 322593f to c96a16e Compare April 28, 2021 20:50

Ngone51 reviewed Apr 29, 2021

View reviewed changes

xkrogen force-pushed the xkrogen-spark-35263-refactor-shuffleblockfetcheriteratorsuite branch from 59137e2 to 596c3dd Compare April 29, 2021 16:21

otterc mentioned this pull request May 5, 2021

[SPARK-32922][SHUFFLE][CORE] Adds support for executors to fetch local and remote merged shuffle data #32140

Closed

Ngone51 reviewed May 8, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala Outdated Show resolved Hide resolved

Ngone51 reviewed May 8, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala Outdated Show resolved Hide resolved

Ngone51 reviewed May 8, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala Outdated Show resolved Hide resolved

xkrogen force-pushed the xkrogen-spark-35263-refactor-shuffleblockfetcheriteratorsuite branch from 596c3dd to 196cb06 Compare May 10, 2021 16:21

otterc reviewed May 11, 2021

View reviewed changes

.idea/vcs.xml Outdated Show resolved Hide resolved

otterc mentioned this pull request May 13, 2021

[SPARK-27991][CORE] Defer the fetch request on Netty OOM #32287

Closed

mridulm reviewed May 14, 2021

View reviewed changes

mridulm approved these changes May 14, 2021

View reviewed changes

xkrogen force-pushed the xkrogen-spark-35263-refactor-shuffleblockfetcheriteratorsuite branch from b2abb87 to 63bf0c4 Compare May 17, 2021 18:04

mridulm approved these changes May 18, 2021

View reviewed changes

otterc reviewed May 18, 2021

View reviewed changes

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala Outdated Show resolved Hide resolved

xkrogen added 11 commits May 18, 2021 10:10

Refactor ShuffleBlockFetcherIteratorSuite -- break out common logic, …

22d824f

…especially instantiating the ShuffleBlockFetcherIterator

Refactor ShuffleBlockFetcherIteratorSuite -- use more Scala-like Mock…

d37adb8

…ito syntax and pull out common calls to when(transfer.fetchBlocks) and verify(transfer, ...).fetchBlocks

Get rid of object, move helper methods into class. Rename verifyFetch…

6aa6364

…BlocksCount to verifyFetchBlocksInvocationCount. Restore helpful comments. Remove one redundant parameter override. Fix upa few minor issues such as unnecessary specification of very long types.

Move 'transfer' to be a field to reduce duplicated mock setup and pas…

9329214

…sing it around in method signatures.

Address ngone51 comments -- minor updates and add a new getBlocksByAd…

4dfe47d

…dress which accepts a Map

Additional refactor to just make getShuffleIteratorWithDefaults direc…

e5d4e34

…tly accept a map of block info

Address Mridul's comments

cc4f3b4

Address Mridul's comments pt 2: Make block list passing more general

b29dc14

Revert part of the changes to address test failure

9a60603

new simplification for the input stream wrapper that doesn't break an…

e3ccd51

…y tests

minor refactor

eea80f5

xkrogen force-pushed the xkrogen-spark-35263-refactor-shuffleblockfetcheriteratorsuite branch from 4fcc6be to eea80f5 Compare May 18, 2021 17:14

Ngone51 approved these changes May 19, 2021

View reviewed changes

asfgit closed this in 186477c May 19, 2021

xkrogen deleted the xkrogen-spark-35263-refactor-shuffleblockfetcheriteratorsuite branch May 19, 2021 16:44

	val iterator = new ShuffleBlockFetcherIterator(
	taskContext,
	transfer,
	blockManager,
	blocksByAddress,
	(_, in) => in,
	48 * 1024 * 1024,
	Int.MaxValue,
	Int.MaxValue,
	Int.MaxValue,
	true,
	false,
	metrics,
	false)

[SPARK-35263] [TEST] Refactor ShuffleBlockFetcherIteratorSuite to reduce duplicated code #32389

[SPARK-35263] [TEST] Refactor ShuffleBlockFetcherIteratorSuite to reduce duplicated code #32389

Uh oh!

Conversation

xkrogen commented Apr 28, 2021

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?

Uh oh!

SparkQA commented Apr 28, 2021

Uh oh!

SparkQA commented Apr 28, 2021

Uh oh!

SparkQA commented Apr 28, 2021

Uh oh!

SparkQA commented Apr 28, 2021

Uh oh!

SparkQA commented Apr 28, 2021

Uh oh!

SparkQA commented Apr 28, 2021

Uh oh!

HyukjinKwon commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ngone51 commented Apr 29, 2021

Uh oh!

xkrogen commented Apr 29, 2021

Uh oh!

SparkQA commented Apr 29, 2021

Uh oh!

SparkQA commented Apr 29, 2021

Uh oh!

SparkQA commented Apr 29, 2021

Uh oh!

xkrogen commented May 4, 2021

Uh oh!

Uh oh!

Uh oh!

Ngone51 May 8, 2021

Choose a reason for hiding this comment

Uh oh!

xkrogen May 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ngone51 commented May 8, 2021

Uh oh!

xkrogen commented May 10, 2021

Uh oh!

SparkQA commented May 10, 2021

Uh oh!

SparkQA commented May 10, 2021

Uh oh!

SparkQA commented May 10, 2021

Uh oh!

otterc commented May 11, 2021

Uh oh!

Uh oh!

mridulm May 14, 2021

Choose a reason for hiding this comment

Uh oh!

mridulm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

mridulm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mridulm left a comment •

edited

Loading