Support shared library build of Redpanda #10220
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The partition recovery manager and topic recovery service are moved from cloud storage into the cluster library. The recovery services make sense to be outside the leaner cloud storage library, and importantly, it breaks a circular dependency between the two subsystems. Signed-off-by: Noah Watkins <[email protected]>
The archival and cluster libraries are intertwined to the point that separating them to resolve a circular dependency has proved quite difficult. On the other hand, compiling archival into the cluster library itself fully resolves the issue. This commit does not move any files, but merely changes the build system to reflect the new compilation strategy. This is useful because there are many inflight changes that would incur a large cost due to a file move and namespace changes. Future clean up should address this temporary layout. Signed-off-by: Noah Watkins <[email protected]>
Primarily this addresses missing dependencies that are required in a shared library build. Signed-off-by: Noah Watkins <[email protected]>
dotnwat
requested review from
jcsp,
Lazin,
BenPope,
rockwotj,
andrwng and
michael-redpanda
rockwotj
reviewed
Apr 20, 2023
There was a problem hiding this comment.
Whoot! This is great 👍
I have a couple questions mostly to make sure I understand.
I was reading your dialogue with Travis about absl::Hash's seed. Did we want to add a test to verify the hash is consistent across .so in this PR or did we want to punt that to another?
I think we should add it separately. Our release/production builds are still static linking. #10231 |
rockwotj
approved these changes
Apr 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Final set of changes needed to enable shared library builds.
Usage:
task rp:build BUILD_TYPE=debug BUILD_SHARED_LIBS=onSee it in action: The CI run for this PR builds with static libraries, as normal. The CI run for this vtools sister PR (https://github.com/redpanda-data/vtools/pull/1726) turns on shared libraries by default for debug builds (in both CI and for normal users).
Once this PR and https://github.com/redpanda-data/vtools/pull/1726 merge users will need need to reconfigure / rebuild all of the redpanda deps the next time they update vtools.
Before
After
Reducing the size of the
bindirectory has a lot of impact: all of the data in there needs to be generated, and in the shared library build we are generating roughly 43x less data and eliminating the linking costs that go into producing that much data. Anecdotally I see that compiling (not linking as was the case before) application.cc and admin_server.cc are now the bottlenecks.For those of you that have tried
moldbut found that it did not work well with the static library build, it may work much better now. Yet to be verified.Backports Required
Release Notes