[improve][broker]Part-2 of PIP-433 add validation when enabling namespace-level Geo-Replication #25170
Conversation
…pace-level Geo-Replication
poorbarcode
requested review from
Technoboy-,
codelipenghui,
coderzc and
lhotari
| || ex instanceof PulsarAdminException.NotFoundException) { | ||
| return null; | ||
| } | ||
| throw new CompletionException(ex); |
There was a problem hiding this comment.
duplicate exception logic, better to add a method for this.
Denovo1998
left a comment
Denovo1998
left a comment
There was a problem hiding this comment.
@poorbarcode
1.Partition consistency verification is "one-way" (only starting from the local topic).
- The current logic will not detect the situation where "the remote already has a topic (and it may conflict with the local one in the future) but the local hasn't been created yet."
- The test does not cover the case where "topic only exists remotely and conflicts with local policy".
2.Performance risk: Calling getPartitionedTopicMetadataAsync remotely once for each topic under the namespace
Should we consider concurrency limiting (semaphore), batching, or state in the document/comments that this is an admin operation and may take time?
| * Validates that the effective auto-topic creation policies are the same between local and remote clusters. | ||
| * The effective policy is computed by: namespace-level policy overrides broker-level if it exists. | ||
| */ | ||
| private CompletableFuture<Void> validateAutoTopicCreationCompatibility(PulsarAdmin remoteAdmin, |
There was a problem hiding this comment.
validateAutoTopicCreationCompatibility(...) does not validate allowAutoTopicCreation?
Should we perhaps include allowAutoTopicCreation in the effective policy and participate in the comparison?
| // Get local broker config | ||
| ServiceConfiguration localConfig = pulsar().getConfiguration(); | ||
| TopicType localBrokerAutoCreationType = localConfig.getAllowAutoTopicCreationType(); | ||
| int localBrokerDefaultPartitions = localConfig.getDefaultNumPartitions(); |
There was a problem hiding this comment.
No matter whether the effective topicType is partitioned or not, will it compare the defaultNumPartitions?
When both sides are effective non-partitioned, but defaultNumPartitions is different, why are they treated as incompatible?
| remoteAdmin.namespaces().getAutoTopicCreationAsync(namespaceStr) | ||
| .exceptionally(ex -> { | ||
| // If namespace doesn't have override, return null | ||
| if (ex.getCause() instanceof PulsarAdminException.NotFoundException |
There was a problem hiding this comment.
Do we need FutureUtil.unwrapCompletionException(e) here?
| * Helper method to clean up a namespace properly. | ||
| * First removes replication clusters (if any), then deletes the namespace. |
There was a problem hiding this comment.
Comment says "then deletes the namespace", the implementation is to reset the replication clusters to a single cluster.
3 participants
Motivation
This is part-2 of PIP-433.
Modifications
Checks compatibility between two clusters when enabling namespace-level replication, which includes the following
Documentation
docdoc-requireddoc-not-neededdoc-complete