Skip to content

Conversation

@jinxing64
Copy link

What changes were proposed in this pull request?

Update the description of spark.shuffle.maxChunksBeingTransferred to include that the new coming connections will be closed when the max is hit and client should have retry mechanism.

@SparkQA
Copy link

SparkQA commented Jul 26, 2017

Test build #79949 has finished for PR 18735 at commit ad710dd.

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

@jinxing64
Copy link
Author

cc @tgravescs @cloud-fan

@cloud-fan
Copy link
Contributor

LGTM

<td>Long.MAX_VALUE</td>
<td>
The max number of chunks allowed to being transferred at the same time on shuffle service.
Note that new coming connections will be closed when the max number is hit. Client should
Copy link
Contributor

Choose a reason for hiding this comment

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

change to say: allowed to be transferred

new coming should be new incoming.

Perhaps we could clarify a bit because Spark has a built in retry mechanism. Could we rephrase to something like: The client will retry according to the shuffle retry configs (see spark.shuffle.io.maxRetries and spark.shuffle.io.retryWait), if those limits are reached the task will fail with fetch failure.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is much better.


/**
* The max number of chunks allowed to being transferred at the same time on shuffle service.
* Note that new coming connections will be closed when the max number is hit. Client should
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as below

The max number of chunks allowed to be transferred at the same time on shuffle service.
Note that new incoming connections will be closed when the max number is hit. The client will
retry according to the shuffle retry configs (see spark.shuffle.io.maxRetries and
spark.shuffle.io.retryWait), if those limits are reached the task will fail with fetch failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for updating, sorry I forgot to ask you to put spark.shuffle.io.maxRetries and spark.shuffle.io.retryWait inside so they show up as references like in other places. See the description of
spark.shuffle.compress
as an example on how ti references other configs.

@tgravescs
Copy link
Contributor

thanks for updating, minor formatting thing I forgot to mention before, otherwise looks good.

@SparkQA
Copy link

SparkQA commented Jul 26, 2017

Test build #79965 has finished for PR 18735 at commit 2216b31.

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

@SparkQA
Copy link

SparkQA commented Jul 26, 2017

Test build #79966 has finished for PR 18735 at commit 14f857e.

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

@jinxing64
Copy link
Author

@tgravescs
Thanks, I should be more careful :)

@SparkQA
Copy link

SparkQA commented Jul 27, 2017

Test build #79983 has finished for PR 18735 at commit 7db7cc7.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in cfb25b2 Jul 27, 2017
@tgravescs
Copy link
Contributor

thanks for fixing @jinxing64

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.

4 participants