ct/frontend: co_await in do_load_slice#30176
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates cloud topics frontend log readers so that exceptions originating from read_some() are caught by the local try/catch in do_load_slice(), by converting the call path to use C++ coroutines (co_await/co_return).
Changes:
- Update
level_zero_log_reader_impl::do_load_slice()toco_awaittheread_some()future so exceptions are handled by the surroundingtry/catch. - Update
level_one_log_reader_impl::do_load_slice()similarly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/v/cloud_topics/level_zero/frontend_reader/level_zero_reader.cc | Switch do_load_slice() to co_return co_await read_some(...) so read_some exceptions are caught/logged before rethrow. |
| src/v/cloud_topics/level_one/frontend_reader/level_one_reader.cc | Same coroutine conversion in do_load_slice() to ensure exception handling works as intended. |
Retry command for Build#83191please wait until all jobs are finished before running the slash command |
nvartolomei
force-pushed
the
nv/ct-fix-do-load-slice-coroutine
branch
from
48f7166 to
3cbd0d6
Compare
Retry command for Build#83203please wait until all jobs are finished before running the slash command |
nvartolomei
force-pushed
the
nv/ct-fix-do-load-slice-coroutine
branch
from
3cbd0d6 to
a6265a5
Compare
| } catch (...) { | ||
| vlog( | ||
| _log.error, "Reader caught exception: {}", std::current_exception()); | ||
| _log.debug, "Reader caught exception: {}", std::current_exception()); |
There was a problem hiding this comment.
why not warn if not a shutdown exception?
There was a problem hiding this comment.
because it would be too spammy during normal work because of throws like this
do you have suggestions how to get best of both worlds?
There was a problem hiding this comment.
Looks like those log lines around the exception throws are already at WARN and ERROR, so I don't see an issue with logging 2x on exceptions in this path? I also don't think we expect these to be firing to a spammy degree during normal operations right?
dotnwat
left a comment
dotnwat
left a comment
There was a problem hiding this comment.
nice catch! pr lgtm but the changes to the log level handling shutdown scenarios isn't explained afaict.
|
@nvartolomei should we get this merged (after addressing the logging?) |
|
Yeh, would be good to merge |
Use co_return/co_await instead of plain return so that exceptions from read_some are caught by the surrounding try/catch block. Log at debug level since exceptions here are not necessarily software errors — the caller may have triggered an abort source.
nvartolomei
force-pushed
the
nv/ct-fix-do-load-slice-coroutine
branch
from
a6265a5 to
3d180b8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| : ss::log_level::warn, | ||
| "Reader caught exception: {}", | ||
| ex); | ||
| set_end_of_stream(); |
There was a problem hiding this comment.
I just noticed. This also didn't call set_end_of_stream() 🤔
Use co_return/co_await instead of plain return so that exceptions from read_some are caught by the surrounding try/catch block.
Backports Required
Release Notes