-
Notifications
You must be signed in to change notification settings - Fork 589
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
cloud_storage: Remove stuck reader exception #12471
Conversation
The exception is no longer used and therefore there is no need to handle it. The EOF condition is handled differently and the transition to the next segment can no longer get stuck because the reader caches the base_offset of the next segment.
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.
LGTM
src/v/ssx/task_local_ptr.h
Outdated
|
||
template<class... Args> | ||
explicit task_local(std::in_place_t, Args... args) | ||
: _val(std::in_place, args...) |
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.
should be Args&&... args and std::forward(args)...
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.
fixed
978649c
to
76f4dc9
Compare
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.
lgtm
Add c-tor overload that takes std::in_place_t and list of parameters for the underlying type.
76f4dc9
to
3695911
Compare
Remove unused exception and the code that handles it. This PR also includes some followup changes from previous PR:
remote_partition
usewith_manifest
to callinitialize_reader_state
to avoid potential race condition.task_local
add new c-tor that acceptsstd::in_place_t
+ list of parameters to construct underlying value.Backports Required
Release Notes