[broker] Setup pulsar cluster with MetadataStore #10600
Conversation
| @@ -308,6 +324,32 @@ static void createTenantIfAbsent(ZooKeeper configStoreZk, String tenant, String | |||
| } | |||
| } | |||
|
|
|||
| static void createTenantIfAbsent(MetadataStore configStore, String tenant, String cluster) throws IOException, | |||
There was a problem hiding this comment.
Does this method https://github.com/apache/pulsar/pull/10600/files#diff-03a1201aab4aa284723e575a7c9b18ad9542fcc02beb80d83cfb87d64adf54b5R300 can be removed?
There was a problem hiding this comment.
As @BewareMyPower pointed out, these methods are still used in PulsarInitialNamespaceSetup and PulsarTransactionCoordinatorMetadataSetup. I plan to open more PRs to fix these commands as well and then remove these methods altogether.
| @@ -338,6 +380,32 @@ static void createNamespaceIfAbsent(ZooKeeper configStoreZk, NamespaceName names | |||
| } | |||
| } | |||
|
|
|||
| static void createNamespaceIfAbsent(MetadataStore configStore, NamespaceName namespaceName, String cluster) | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
It's referenced in PulsarInitialNamespaceSetup, I think it's better to open a new PR for setup namespace with MetadataStore API.
There was a problem hiding this comment.
Yes, as I explained above.
codelipenghui
requested review from
merlimat,
eolivelli,
addisonj,
congbobo184,
gaoran10,
sijie and
zymap
| return FutureUtil.failedFuture(e); | ||
| } | ||
| } | ||
| return CompletableFuture.completedFuture(null); |
There was a problem hiding this comment.
IMO there's no need to return a future because it's not an asynchronous API.
There was a problem hiding this comment.
I mean MetadataStoreLifecycle#initializeCluster should not be an asynchronous API. Otherwise it should be renamed to initializeClusterAsync and the associated synchronous API initializeCluster needs to be added as well.
There was a problem hiding this comment.
Understood. It seems MetadataStore APIs are following a different pattern: all IO operations are asynchronous by default. So I think MetadataStoreLifecycle#initializeCluster should be designed in the same way.
There was a problem hiding this comment.
Apart from the creating of the ZooKeeper client you could use all ZK async API calls in order to make it fully async.
But I believe it is overkilling, as you usually are going to wait for this setup operation to complete in real world operations.
I am fine to keep using CompletableFuture, it makes sense to me, for consistency as @fantapsody said
There was a problem hiding this comment.
I agree with @eolivelli that if you're going to design an asynchronous API, it's better keep using CompletableFuture. I suggested using a synchronous API here just because your implementation is a synchronous way @fantapsody
There was a problem hiding this comment.
It's true. The synchronous implementation is because I want to keep this piece of code simple and reuse the previous code, and the asynchronous API definition is to leave room for future optimizations if needed.
| return FutureUtil.failedFuture(e); | ||
| } | ||
| } | ||
| return CompletableFuture.completedFuture(null); |
There was a problem hiding this comment.
Apart from the creating of the ZooKeeper client you could use all ZK async API calls in order to make it fully async.
But I believe it is overkilling, as you usually are going to wait for this setup operation to complete in real world operations.
I am fine to keep using CompletableFuture, it makes sense to me, for consistency as @fantapsody said
### Motivation Refactor the pulsar cluster setup command to use the new `MetadataStore` API. ### Modifications - Refactored `PulsarClusterMetadataSetup` command to use the new `MetadataStore` API. - Added a new `MetadataStoreLifecycle` interface for the `MetadataStore` implementations to do initialization as needed. - For zookeeper, it creates the root node in chroot mode
### Motivation Refactor the pulsar cluster setup command to use the new `MetadataStore` API. ### Modifications - Refactored `PulsarClusterMetadataSetup` command to use the new `MetadataStore` API. - Added a new `MetadataStoreLifecycle` interface for the `MetadataStore` implementations to do initialization as needed. - For zookeeper, it creates the root node in chroot mode
Motivation
Refactor the pulsar cluster setup command to use the new
MetadataStoreAPI.Modifications
PulsarClusterMetadataSetupcommand to use the newMetadataStoreAPI.MetadataStoreLifecycleinterface for theMetadataStoreimplementations to do initialization as needed.Verifying this change
This change is already covered by existing tests, such as
ClusterMetadataSetupTest.Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation