mridulm commented on code in PR #37922:
URL: https://github.com/apache/spark/pull/37922#discussion_r1067551485
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockStoreClient.java:
##########
@@ -256,6 +256,22 @@ public void onFailure(Throwable e) {
}
}
+ @Override
+ public boolean removeShuffleMerge(String host, int port, int shuffleId, int
shuffleMergeId) {
+ checkInit();
+ try {
+ TransportClient client = clientFactory.createClient(host, port);
+ client.send(
+ new RemoveShuffleMerge(appId, comparableAppAttemptId, shuffleId,
shuffleMergeId)
Review Comment:
Failure to remove blocks in `removeBlocks` has an impact on resource
utilization (for example, block could be in memory). Here, it is more of a best
effort cleanup - and can fail in context of decommissioned nodes, etc - log
pollution is something I am concerned with here.
Having said that - the exact same problem exists for
`removeShuffle.removeShuffleFromShuffleServicesFutures` unfortunately - and
that is an issue : I would argue for reducing that log message from `warn` to
`info` (or even `debug` ) in `ExternalBlockStoreClient.removeBlocks` when
failing to remove shuffle files from ESS.
We can make changes here to log in `info` - and make changes to
`removeBlocks` for ESS block removal later.
Thoughts ?
+CC @tgravescs who reviewed #35085
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
