-
Notifications
You must be signed in to change notification settings - Fork 552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remote: fix exception checking in delete_objects_sequentially #17559
remote: fix exception checking in delete_objects_sequentially #17559
Conversation
Catching the exception with `const std::exception&` as the type forces the compiler to understand the error's type as `std::exception`, and trips up our shutdown-error-checking[1]. This change updates the code to pass the `std::current_exception()` `exception_ptr` directly to `is_shutdown_exception()`. This previously showed up as an error log: ``` cloud_storage - [fiber28~216|0|29845ms] - remote.cc:1293 - Failed to delete keys: Sleep is aborted ``` Fixes redpanda-data#17506 [1] https://godbolt.org/z/xqeabz35r
f2a143b
to
7c09f37
Compare
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/47249#018ea14d-02d8-4e53-a070-a0cf1a83a8cd |
}); | ||
std::vector<upload_result> results; | ||
results.reserve(key_nodes.size()); | ||
auto fut = co_await ss::coroutine::as_future(ss::max_concurrent_for_each( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know about coroutine::as_future
, this is really nice
results.push_back(result); | ||
}); | ||
}) | ||
.handle_exception_type([ctxlog](const std::exception& ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to switching to coroutine. Just so I understand, this fix could also have been to change .handle_exception_type([ctxlog](const std::exception& ex)
to .handle_exception([ctxlog](std::exception_ptr eptr)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha 🤦, yea I think so
Catching the exception with
const std::exception&
as the type forces the compiler to understand the error's type asstd::exception
, and trips up our shutdown-error-checking[1].This change updates the code to pass the
std::current_exception()
exception_ptr
directly tois_shutdown_exception()
.This previously showed up as an error log:
Fixes #17506
[1] https://godbolt.org/z/xqeabz35r
Backports Required
Release Notes