Construct and inject bookie components #2901
Merged
Conversation
This file contains hidden or 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
eolivelli
requested changes
Nov 17, 2021
Contributor
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Great work
I left some feedback, PTAL
...-allocator/src/main/java/org/apache/bookkeeper/common/allocator/ByteBufAllocatorBuilder.java
Show resolved
Hide resolved
|
|
||
| private synchronized void closePreAllocateLog() { | ||
| if (preallocatedLogId != -1) { | ||
| if (preallocation != null) { |
Contributor
There was a problem hiding this comment.
this change looks unrelated
Contributor
Author
There was a problem hiding this comment.
Yeah, that was causing a test to fail so I fixed it.
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
Outdated
Show resolved
Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataDrivers.java
Outdated
Show resolved
Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/PortManager.java
Show resolved
Hide resolved
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
Show resolved
Hide resolved
Vanlightly
force-pushed
the
inject-components
branch
4 times, most recently
from
eba8840 to
80ca326
Compare
Construct and inject metadata driver and ledger storage outside of the BookieImpl. Also extract the cookie validation into a separate injectable interface. The metadata driver is a factory that creates the correct metadata accessors based on the configuration of the bookie. This is used by multiple components, so should be constructed before these components and passed in as a parameter. Previously it was constructed by the bookie, and other components called into the bookie to get the instance, which broken encapsulation of the bookie and let to a bunch of ugly mocking. This change also removes a circular dependency in the construction of the RegistrationManager. Previously it was requiring a listener on construction, but the listener needed the bookie instance, so it all went through a supplier. Now the listener is not needed at construction, but rather it is registered then the bookie starts. This change also adds an injectable interface for cookie validation. The legacy implementation of this interface is then injected into the BookieImpl on creation. The legacy implementation is code moved out of the BookieImpl class itself. Injecting ledger storage is good for encapsulation, as it stops the ledgerstorage depending on bookie behaviour. To that end, this change highlights a couple of circular dependencies. Notably, between the ledger storage and each of the statemanager, checkpointer and checkpointState. I've removed each of these from LedgerStorage#initialize() so that is is clearer that these are circular, and so that they can be fixed one by one. One thing to note in this change is that the order of initialization has changed. LedgerStorage is initialized before the cookie check. This means that directory structure needs to be checked before the cookie check. Previously it was done as part of the cookie check. Ledger storage must be initialized before the cookie check, as the cookie check needs to use ledger storage to update limbo bits. BookieImpl is still responsible for shutting down the LedgerStorage. I would have changed this, but it would have meant a lot of changes, as there are good few ledger storage implementations.
Some tests in distributedlog require the ability to scale up and down bookies, using the LocalBookKeeper class but doing so using BookieServer directly. Now that components are constructed outside of BookieServer, this scaling had to be built into LocalBookKeeper itself so the components could be constructed and destroyed correctly.
Vanlightly
force-pushed
the
inject-components
branch
from
520ba29 to
cc24a37
Compare
Contributor
Author
|
@eolivelli @merlimat @ivankelly all tests are passing now. PTAL. |
Contributor
|
Merged |
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
* Construct and inject bookie components Construct and inject metadata driver and ledger storage outside of the BookieImpl. Also extract the cookie validation into a separate injectable interface. The metadata driver is a factory that creates the correct metadata accessors based on the configuration of the bookie. This is used by multiple components, so should be constructed before these components and passed in as a parameter. Previously it was constructed by the bookie, and other components called into the bookie to get the instance, which broken encapsulation of the bookie and let to a bunch of ugly mocking. This change also removes a circular dependency in the construction of the RegistrationManager. Previously it was requiring a listener on construction, but the listener needed the bookie instance, so it all went through a supplier. Now the listener is not needed at construction, but rather it is registered then the bookie starts. This change also adds an injectable interface for cookie validation. The legacy implementation of this interface is then injected into the BookieImpl on creation. The legacy implementation is code moved out of the BookieImpl class itself. Injecting ledger storage is good for encapsulation, as it stops the ledgerstorage depending on bookie behaviour. To that end, this change highlights a couple of circular dependencies. Notably, between the ledger storage and each of the statemanager, checkpointer and checkpointState. I've removed each of these from LedgerStorage#initialize() so that is is clearer that these are circular, and so that they can be fixed one by one. One thing to note in this change is that the order of initialization has changed. LedgerStorage is initialized before the cookie check. This means that directory structure needs to be checked before the cookie check. Previously it was done as part of the cookie check. Ledger storage must be initialized before the cookie check, as the cookie check needs to use ledger storage to update limbo bits. BookieImpl is still responsible for shutting down the LedgerStorage. I would have changed this, but it would have meant a lot of changes, as there are good few ledger storage implementations. * Ensure RegistrationManagers are closed * Remove PortManager next port and args that set it * Fix checkstyle in tests * Add stacktrace to standalone test * Add --debug to standalone test * Add dynamic scaling to LocalBookKeeper Some tests in distributedlog require the ability to scale up and down bookies, using the LocalBookKeeper class but doing so using BookieServer directly. Now that components are constructed outside of BookieServer, this scaling had to be built into LocalBookKeeper itself so the components could be constructed and destroyed correctly. * Moved state store server creation to constructor * Remove stacktrace option from standalone test Co-authored-by: Ivan Kelly <ikelly@splunk.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR is a set of refactoring changes that are required before the "running without the journal" changes can be submitted. I apologise for the size of this PR but due to the refactorings involved, many classes and many tests were impacted. Functionality is basically the same, the changes are around when and where components are constructed and their injection.
Motivation
Simplify and centralize the construction of bookie components outside of the BookieImpl. By constructing all dependencies and injecting them into the places required such as BookieImpl, we simplify this process and avoid circular dependencies. Many of these dependencies are added to the lifecycle component stack which cleanly handles their lifetimes.
Also we extract some functionality from BookieImpl into interface/impl, such as cookie validation.
Changes
The metadata driver is a factory that creates the correct metadata accessors based on the configuration of the bookie. This is used by multiple components, so should be constructed before these components and passed in as a parameter.
Previously it was constructed by the bookie, and other components called into the bookie to get the instance, which broken encapsulation of the bookie and led to a bunch of ugly mocking.
This change also removes a circular dependency in the construction of the RegistrationManager. Previously it was requiring a listener on construction, but the listener needed the bookie instance, so it all went through a supplier. Now the listener is not needed at construction, but rather it is registered then the bookie starts.
This change also adds an injectable interface for cookie validation. The legacy implementation of this interface is then injected into the BookieImpl on creation. The legacy implementation is code moved out of the BookieImpl class itself.
Injecting ledger storage is good for encapsulation, as it stops the ledgerstorage depending on bookie behaviour. To that end, this change highlights a couple of circular dependencies. Notably, between the ledger storage and each of the statemanager, checkpointer and checkpointState. I've removed each of these from LedgerStorage#initialize() so that it is clearer that these are circular, and so that they can be fixed one by one.
Many the components are now shutdown as lifecycle components, rather than by the BookieImpl. The new AutoCloseableLifecycleComponent is provided to allow classes to be added as lifecycle components and their shutdown handled for them. BookieImpl is still responsible for shutting down the LedgerStorage. I would have changed this, but it would have meant a lot of changes, as there are good few ledger storage implementations.
Because components get constructed outside of the BookieImpl, some changes to the tests were required such as introducing the LocalBookie class that would perform these duties in tests.