[ML] Make ManagedLedger storage configurable #9397
Conversation
*Motivation* This is the first step to allow supporting different storage implementations for Pulsar
| * | ||
| * @return the default bookkeeper client. | ||
| */ | ||
| BookKeeper getBookKeeperClient(); |
There was a problem hiding this comment.
If we want to support different storage, why the interface contains the bookkeeper client related method, seems this should be handled in the implementation?
There was a problem hiding this comment.
Probably this interface should be org.apache.bookkeeper.client.api.Bookkeeper (the new BK API) if we want to look forward for the future.
I know that this is not possible with the current codebase, that is bound to the old API.
So this comment is not a request to change but just a reminder for the future.
There was a problem hiding this comment.
@codelipenghui This is the first attempt to provide an interface. It is a bit difficult to hide everything at this moment. Hence I marked the interface as private and unstable. So we can iterate changes over this interface.
There was a problem hiding this comment.
@eolivelli see reasons above.
There was a problem hiding this comment.
sure @sijie
I had already approved this patch.
please go ahead
There was a problem hiding this comment.
Need approval from @codelipenghui and @315157973
| * @param bookkeperProvider bookkeeper provider | ||
| * @throws Exception | ||
| */ | ||
| void initialize(ServiceConfiguration conf, |
There was a problem hiding this comment.
If we wrap an object or abstract a client API, which has nothing to do with the storage medium, would it be better?
If it is abstract, we don’t have to use zk or bk.
In the future, we will increase the storage type, only need to modify this object, and no need to modify the interface.
There was a problem hiding this comment.
@315157973 see my comment in #9397 (comment)
| * | ||
| * @return the default bookkeeper client. | ||
| */ | ||
| BookKeeper getBookKeeperClient(); |
There was a problem hiding this comment.
Probably this interface should be org.apache.bookkeeper.client.api.Bookkeeper (the new BK API) if we want to look forward for the future.
I know that this is not possible with the current codebase, that is bound to the old API.
So this comment is not a request to change but just a reminder for the future.
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
This version update is convenient for tests in real environment since there's no binary download url for original pulsar `2.8.0-rc-202101252233`. This PR fixes the API incompatibility problems that are introduced by apache/pulsar#9397 and apache/pulsar#9302. Another significant change between these two versions is apache/pulsar#9338, which introduced metadata-store API for cluster resources. This PR fixed the test failure caused by it as well. Since KoP `tests` module only uses one `MockZooKeeper` to manage z-nodes, see `KopProtocolHandlerTestBase#createMockZooKeeper`, the mocked `createConfigurationMetadataStore` method returns `mockedZooKeeper` here instead of a `mockedZooKeeperGlobal` like what Pulsar did in `MockedPulsarServiceBaseTest`. Besides, there's a test bug in `testBrokerHandleTopicMetadataRequest` that was not exposed by the previous Pulsar. This PR fixes it.
Motivation
This is the first step to allow supporting different storage implementations for Pulsar