You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/12/08 04:42:28 UTC
[GitHub] [spark] pan3793 opened a new pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
pan3793 opened a new pull request #34831:
URL: https://github.com/apache/spark/pull/34831
### What changes were proposed in this pull request?
Simplify code of fetchBlocks w/o retry
### Why are the changes needed?
Simplify code.
The original code added in [SPARK-4188], the `RetryingBlockTransferor` should be stable.
#33340 renames `RetryingBlockFetcher` to `RetryingBlockTransferor`, the comments still calls it as `Fetcher`, it's a little misleading.
```
if (maxRetries > 0) {
// Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
// a bug in this code. We should remove the if statement once we're sure of the stability.
new RetryingBlockTransferor(transportConf, blockFetchStarter, blockIds, listener).start()
} else {
blockFetchStarter.createAndStart(blockIds, listener)
}
```
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing UTs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #34831:
URL: https://github.com/apache/spark/pull/34831
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #34831:
URL: https://github.com/apache/spark/pull/34831#discussion_r767473622
##########
File path: core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala
##########
@@ -139,14 +139,7 @@ private[spark] class NettyBlockTransferService(
}
}
}
-
- if (maxRetries > 0) {
- // Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
- // a bug in this code. We should remove the if statement once we're sure of the stability.
Review comment:
When `spark.shuffle.io.maxRetries=0`, it tests `OneForOneBlockFetcher` rather than `RetryingBlockTransferor`, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34831:
URL: https://github.com/apache/spark/pull/34831#issuecomment-988497551
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #34831:
URL: https://github.com/apache/spark/pull/34831#issuecomment-1083833163
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #34831:
URL: https://github.com/apache/spark/pull/34831#discussion_r767450865
##########
File path: core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala
##########
@@ -139,14 +139,7 @@ private[spark] class NettyBlockTransferService(
}
}
}
-
- if (maxRetries > 0) {
- // Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
- // a bug in this code. We should remove the if statement once we're sure of the stability.
Review comment:
There are several place overwrite the conf to `0`, please search `("spark.shuffle.io.maxRetries", "0")` in `ExternalShuffleIntegrationSuite` and `BlockManagerSuite`
##########
File path: core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala
##########
@@ -139,14 +139,7 @@ private[spark] class NettyBlockTransferService(
}
}
}
-
- if (maxRetries > 0) {
- // Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
- // a bug in this code. We should remove the if statement once we're sure of the stability.
Review comment:
There are several places overwrite the conf to `0`, please search `("spark.shuffle.io.maxRetries", "0")` in `ExternalShuffleIntegrationSuite` and `BlockManagerSuite`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #34831:
URL: https://github.com/apache/spark/pull/34831#discussion_r767686871
##########
File path: core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala
##########
@@ -139,14 +139,7 @@ private[spark] class NettyBlockTransferService(
}
}
}
-
- if (maxRetries > 0) {
- // Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
- // a bug in this code. We should remove the if statement once we're sure of the stability.
Review comment:
Updated UT, please take a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #34831:
URL: https://github.com/apache/spark/pull/34831#discussion_r767447666
##########
File path: core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala
##########
@@ -139,14 +139,7 @@ private[spark] class NettyBlockTransferService(
}
}
}
-
- if (maxRetries > 0) {
- // Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
- // a bug in this code. We should remove the if statement once we're sure of the stability.
Review comment:
I wonder how do you prove this since `maxRetries` default to 3 rather than 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #34831: [SPARK-37574][CORE][SHUFFLE] Simplify fetchBlocks w/o retry
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #34831:
URL: https://github.com/apache/spark/pull/34831#discussion_r767497436
##########
File path: core/src/main/scala/org/apache/spark/network/netty/NettyBlockTransferService.scala
##########
@@ -139,14 +139,7 @@ private[spark] class NettyBlockTransferService(
}
}
}
-
- if (maxRetries > 0) {
- // Note this Fetcher will correctly handle maxRetries == 0; we avoid it just in case there's
- // a bug in this code. We should remove the if statement once we're sure of the stability.
Review comment:
Sound like add unit test cases in `RetryingBlockTransferorSuite` is more reasonable, will update soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org