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 2022/03/29 09:27:03 UTC
[GitHub] [spark] weixiuli opened a new pull request #36000: [SPARK-38683][SHUFFLE] [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
weixiuli opened a new pull request #36000:
URL: https://github.com/apache/spark/pull/36000
### What changes were proposed in this pull request?
It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
### Why are the changes needed?
To reduce I/O operations and improve performance for the External Shuffle Service.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing unittests.
--
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] mridulm commented on pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #36000:
URL: https://github.com/apache/spark/pull/36000#issuecomment-1082626193
The change look good to me, but I would like to ensure there are more eyes on this.
+CC @Ngone51, @attilapiros
--
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 #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838294518
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -51,6 +51,12 @@
// The channel associated to the stream
final Channel associatedChannel;
+ // Indicates whether the buffers is materialized, some buffers like
+ // ShuffleManagedBufferIterator, ShuffleChunkManagedBufferIterator, ManagedBufferIterator are
+ // materialized only when the iterator is requested by the next method, so we should not
+ // request and release them, to reduce I/O operations and improve performance for the External
+ // Shuffle Service.
Review comment:
```suggestion
// Indicates whether the buffers is only materialized when next() is called. Some buffers like
// ShuffleManagedBufferIterator, ShuffleChunkManagedBufferIterator, ManagedBufferIterator are
// not materialized until the iterator is traversed by calling next(). We use it to decide whether buffers
// should be released at connectionTerminated() in order to avoid unnecessary buffer materialization,
// which could be I/O based.
```
--
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] weixiuli commented on a change in pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838266412
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -51,6 +51,7 @@
// The channel associated to the stream
final Channel associatedChannel;
+ final boolean shouldRelease;
Review comment:
Done. @Ngone51
--
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 #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838277850
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockHandler.java
##########
@@ -157,14 +157,14 @@ protected void handleMessage(
iterator = new ShuffleChunkManagedBufferIterator((FetchShuffleBlockChunks) msgObj);
}
streamId = streamManager.registerStream(client.getClientId(), iterator,
- client.getChannel());
+ client.getChannel(), false);
Review comment:
I think you should flip the value after renaming.
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockHandler.java
##########
@@ -157,14 +157,14 @@ protected void handleMessage(
iterator = new ShuffleChunkManagedBufferIterator((FetchShuffleBlockChunks) msgObj);
}
streamId = streamManager.registerStream(client.getClientId(), iterator,
- client.getChannel());
+ client.getChannel(), false);
} else {
// For the compatibility with the old version, still keep the support for OpenBlocks.
OpenBlocks msg = (OpenBlocks) msgObj;
numBlockIds = msg.blockIds.length;
checkAuth(client, msg.appId);
streamId = streamManager.registerStream(client.getClientId(),
- new ManagedBufferIterator(msg), client.getChannel());
+ new ManagedBufferIterator(msg), client.getChannel(), false);
Review comment:
ditto
--
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] weixiuli commented on pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on pull request #36000:
URL: https://github.com/apache/spark/pull/36000#issuecomment-1082578450
> Nice improvement ! Add test to actually validate that there are no further calls to next once closed ?
Done. Thanks for your review @mridulm
--
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 #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838140176
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -51,6 +51,7 @@
// The channel associated to the stream
final Channel associatedChannel;
+ final boolean shouldRelease;
Review comment:
How about we rename it to "isBufferMaterializedOnNext" and add some comments to explain? I think it's more clear to tell why we don't release buffers if they're not materialized yet.
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -59,10 +60,12 @@
// Used to keep track of the number of chunks being transferred and not finished yet.
final AtomicLong chunksBeingTransferred = new AtomicLong(0L);
- StreamState(String appId, Iterator<ManagedBuffer> buffers, Channel channel) {
+ StreamState(String appId, Iterator<ManagedBuffer> buffers, Channel channel,
+ boolean shouldRelease) {
Review comment:
```suggestion
StreamState(
String appId,
Iterator<ManagedBuffer> buffers,
Channel channel,
boolean shouldRelease) {
```
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -215,12 +218,17 @@ public long chunksBeingTransferred() {
* to be the only reader of the stream. Once the connection is closed, the stream will never
* be used again, enabling cleanup by `connectionTerminated`.
*/
- public long registerStream(String appId, Iterator<ManagedBuffer> buffers, Channel channel) {
+ public long registerStream(String appId, Iterator<ManagedBuffer> buffers, Channel channel,
+ boolean shouldRelease) {
Review comment:
```suggestion
public long registerStream(
String appId,
Iterator<ManagedBuffer> buffers,
Channel channel,
boolean shouldRelease) {
```
--
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] weixiuli commented on a change in pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838287157
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -130,7 +141,7 @@ public void connectionTerminated(Channel channel) {
try {
// Release all remaining buffers.
- while (state.buffers.hasNext()) {
+ while (state.isBufferMaterializedOnNext && state.buffers.hasNext()) {
Review comment:
Done.
--
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 closed pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
Ngone51 closed pull request #36000:
URL: https://github.com/apache/spark/pull/36000
--
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 #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #36000:
URL: https://github.com/apache/spark/pull/36000#issuecomment-1083231831
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] Ngone51 commented on a change in pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838278930
##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -130,7 +141,7 @@ public void connectionTerminated(Channel channel) {
try {
// Release all remaining buffers.
- while (state.buffers.hasNext()) {
+ while (state.isBufferMaterializedOnNext && state.buffers.hasNext()) {
Review comment:
Shouldn't it be `!state.isBufferMaterializedOnNext`?
--
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] mridulm commented on pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #36000:
URL: https://github.com/apache/spark/pull/36000#issuecomment-1082028285
+CC @otterc, @Ngone51
--
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 pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #36000:
URL: https://github.com/apache/spark/pull/36000#issuecomment-1085329963
Thanks, merged to Master!
--
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