Skip to content
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

[storage]: remove storage::log pimpl wrapper #12544

Merged
merged 6 commits into from
Aug 3, 2023

Conversation

dotnwat
Copy link
Member

@dotnwat dotnwat commented Aug 2, 2023

Last quarter we remove the in-memory implementation, but then we were left with the pimpl wrapper and only one implementation. This PR removes the pimpl wrapper.

  • pimpl is fun, and very cool, but clangd can't see through it which is a bummer.
  • we only have disk implementation, but kept the abstraction layer because it is nice to not export the entire implementation out of storage layer.
  • updating multiple interfaces and punching through with pimpl was pure overhead with no gain.
  • ss::shared_ptr<log> vs using log_ptr = ss::shared_ptr<log> - used the former because (1) explicit is nice and (2) outside of tests (ie core redpanda) there are only a handful of places where it is spelled out explicitly such as method parameters.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

  • none

@dotnwat dotnwat marked this pull request as draft August 2, 2023 06:26
dotnwat added 4 commits August 2, 2023 12:31
Switches the tree to operate on ss::shared_ptr<log>. This is a purely
mechnical preparation for the next commit which will remove the pimpl
wrapper around. Making this change first means that the two
modifications do not need to be in the same commit. The oddities that
arise (e.g. optional<shared_ptr>) will be cleaned-up in a subsequent
commit.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
The get_impl wrapper is removed in the next commit as it is now
unnecessary.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Now that holders of an ss::shared_ptr alreayd have a polymorphic pointer
to the underlying implementation there is no need for an extra accessor.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Now a nullptr can serve the same purpose.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
@dotnwat dotnwat changed the title testing no log pimpl [storage]: remove storage::log pimpl wrapper Aug 2, 2023
dotnwat added 2 commits August 2, 2023 12:37
Not strictly necessary, but a nice safety net since the only users of
the log should be holding a shared pointer anyway.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
@dotnwat
Copy link
Member Author

dotnwat commented Aug 2, 2023

@dotnwat dotnwat marked this pull request as ready for review August 2, 2023 22:00
Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

@dotnwat dotnwat merged commit d200930 into redpanda-data:dev Aug 3, 2023
ztlpn added a commit that referenced this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants