Skip to content

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented May 26, 2017

What changes were proposed in this pull request?

This PR includes some minor improvement for the comments and tests in #16989

How was this patch tested?

N/A

@cloud-fan
Copy link
Contributor Author

cc @jinxing64

@SparkQA
Copy link

SparkQA commented May 26, 2017

Test build #77415 has started for PR 18117 at commit 510c195.

@cloud-fan
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented May 26, 2017

Test build #77416 has finished for PR 18117 at commit 510c195.

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

@cloud-fan
Copy link
Contributor Author

retest this please

// TODO: Encryption and compression should be considered.
// Fetch remote shuffle blocks to disk when the request is too large. Since the shuffle data is
// already encrypted and compressed over the wire(w.r.t. the related configs), we can just fetch
// the data and write it to file directly.

Choose a reason for hiding this comment

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

I think this change is really good. Sorry for my ambiguous.

def fetchShuffleBlock(blocksByAddress: Seq[(BlockManagerId, Seq[(BlockId, Long)])]): Unit = {
// Set `maxBytesInFlight` and `maxReqsInFlight` to `Int.MaxValue`, so that during the
// construction of `ShuffleBlockFetcherIterator`, all requests to fetch remote shuffle blocks
// are issued. The `maxReqSizeShuffleToMem` is hard-coded as 200 here.

Choose a reason for hiding this comment

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

👍

@jinxing64
Copy link

@cloud-fan
Thanks a lot for notification. I think it's really good change here 👍

@SparkQA
Copy link

SparkQA commented May 26, 2017

Test build #77425 has finished for PR 18117 at commit 510c195.

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

@cloud-fan
Copy link
Contributor Author

thanks for the review, merging to master/2.2!

asfgit pushed a commit that referenced this pull request May 27, 2017
…read

## What changes were proposed in this pull request?

This PR includes some minor improvement for the comments and tests in #16989

## How was this patch tested?

N/A

Author: Wenchen Fan <[email protected]>

Closes #18117 from cloud-fan/follow.

(cherry picked from commit 1d62f8a)
Signed-off-by: Wenchen Fan <[email protected]>
@asfgit asfgit closed this in 1d62f8a May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants