HuaHuaY commented on code in PR #649:
URL: https://github.com/apache/iceberg-cpp/pull/649#discussion_r3370637134
##########
src/iceberg/update/expire_snapshots.cc:
##########
@@ -99,19 +105,48 @@ class FileCleanupStrategy {
}
/// \brief Delete files at the given locations.
+ ///
+ /// Best-effort: errors are suppressed to mirror Java's
suppressFailureWhenFinished.
+ /// When a custom delete function was provided, deletes are invoked one path
at a time,
+ /// optionally parallelized via the strategy's executor. Otherwise the
FileIO bulk
+ /// `DeleteFiles` API is invoked once with a bounded retry that stops on
kNotFound.
void DeleteFiles(const std::unordered_set<std::string>& paths) {
- try {
- if (delete_func_) {
- for (const auto& path : paths) {
+ if (paths.empty()) return;
+ std::vector<std::string> path_list(paths.begin(), paths.end());
+
+ if (!delete_func_) {
+ // Bulk path: rely on FileIO::DeleteFiles. The tight retry helps
atomic-bulk
+ // implementations (e.g. an S3 DeleteObjects-backed FileIO) ride out a
+ // transient throttle or network blip on the single round trip.
+ //
+ // Caveat: for the default fail-fast iterative impl, a retried attempt
+ // re-submits the full path_list, so files already deleted on a prior
+ // attempt come back as kNotFound and short-circuit the retry (kNotFound
is
+ // in StopRetryOn). That is best-effort cleanup -- still no worse than
the
+ // prior behaviour of a single un-retried call -- and we discard the
final
+ // Status to match Java's suppressFailureWhenFinished semantics.
+ RetryRunner<retry::StopRetryOn<ErrorKind::kNotFound>>
runner(kDeleteRetryConfig);
+ std::ignore = runner.Run([&]() { return
file_io_->DeleteFiles(path_list); });
+ return;
+ }
+
+ // Custom callback path: invoke one path at a time, optionally on a worker
thread
+ // pulled from the configured executor. Without an executor TaskGroup runs
the
+ // callbacks synchronously on the calling thread.
+ TaskGroup<> group;
Review Comment:
You can use `TaskGroup<retry::StopRetryOn<ErrorKind::kNotFound>>
group(kDeleteRetryConfig)` to simplify code.
--
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]
