Cloud Storage: Ignore abort_requested_exception on shutdown #16780
Conversation
|
|
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45466#018df1da-da2c-44ec-b654-46afc82ccc1f ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45466#018df1da-da26-43a2-8010-530df1f9409c |
src/v/cloud_storage/remote.cc
Outdated
| .handle_exception_type( | ||
| [](const ss::abort_requested_exception& /* ignore */) {}) |
There was a problem hiding this comment.
nit: it might be useful to still log these, but at debug log level. In digging into CI failures, silently dropped exceptions can be the difference between an obvious root cause or not (note that most CI runs with trace level logging enabled)
There was a problem hiding this comment.
Ahh good point. I advised @jcipar to not log at all (based on ssx::background behavior) but I think you're right.
| @@ -1268,6 +1268,8 @@ ss::future<upload_result> remote::delete_objects_sequentially( | |||
| results.push_back(result); | |||
| }); | |||
| }) | |||
| .handle_exception_type( | |||
| [](const ss::abort_requested_exception& /* ignore */) {}) | |||
| .handle_exception_type([ctxlog](const std::exception& ex) { | |||
There was a problem hiding this comment.
nit: it might not be an issue, but just fyi, check out ssx::is_shutdown_exception -- if other kinds of common shutdown-related errors pop up from this call site it might be worth switching over
There was a problem hiding this comment.
How does this change look?
There was a problem hiding this comment.
This looks great! Thank you
…og at debug level in that case.
| @@ -1268,6 +1268,8 @@ ss::future<upload_result> remote::delete_objects_sequentially( | |||
| results.push_back(result); | |||
| }); | |||
| }) | |||
| .handle_exception_type( | |||
| [](const ss::abort_requested_exception& /* ignore */) {}) | |||
| .handle_exception_type([ctxlog](const std::exception& ex) { | |||
There was a problem hiding this comment.
This looks great! Thank you
| vlog( | ||
| ctxlog.debug, | ||
| "Failed to delete keys during shutdown: {}", | ||
| ex.what()); | ||
|
|
||
| } else { | ||
| vlog(ctxlog.error, "Failed to delete keys: {}", ex.what()); |
There was a problem hiding this comment.
this would be a good place for vlogl, eg
auto log_level = ...;
vlogl(ctxlog, log_level, ...);
but the messages are different
Looked like the partition was being moved and we were letting an abort error leak out.
Backports Required
Release Notes
Fixes: #16726
Fixes: #16635