otterc commented on code in PR #40972:
URL: https://github.com/apache/spark/pull/40972#discussion_r1236244815
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -188,6 +212,31 @@ public void onFailure(Throwable t) {
}
}
+ private <T> void retry(
+ int times,
+ final int maxRetries,
+ int delayMs,
+ Supplier<CompletableFuture<T>> action,
+ CompletableFuture<T> future) {
+ action.get()
+ .thenAccept(future::complete)
+ .exceptionally(e -> {
+ boolean isIOException = e instanceof IOException
+ || (e.getCause() != null && e.getCause() instanceof
IOException);
Review Comment:
This should also include Sasl timeout (see
[here](https://github.com/apache/spark/blob/692ec99bc7884cc998afbf63e4ef53053c0c9dd7/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RetryingBlockTransferor.java#L218)).
In future, if we want to add more exceptions for which there will be a retry,
then we have to be careful to update all the places where this logic is use, so
I think we should come up a with a way that this is not duplicated.
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/BlockStoreClient.java:
##########
@@ -188,6 +212,31 @@ public void onFailure(Throwable t) {
}
}
+ private <T> void retry(
Review Comment:
Can we create a test suite for `BlockStoreClient` with UTs for this. Earlier
this class didn't have much implementation. Since we have added this logic
here, I think we can add UTs for it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
