[fix][broker]The partitions of the topic was wrongly created even though this cluster was not allowed to access it#25443
Conversation
|
/pulsarbot rerun-failure-checks |
…ugh this cluster was not allowed to access it
poorbarcode
force-pushed
the
fix/create_missing_partitions_without_allowed_clusters
branch
from
d3ca28b to
877b734
Compare
| } | ||
|
|
||
| /** | ||
| * @return Triple [namespace policies, global topic policies, topic policies]. |
There was a problem hiding this comment.
Stale copy-paste from getCombinedTopicPolicies — this method returns CompletableFuture<Boolean>, not a Triple.
There was a problem hiding this comment.
Good pointer, I fixed it
| Optional<TopicPolicies> globalTopicP = triple.getMiddle(); | ||
| Optional<Policies> nsPolicies = triple.getLeft(); | ||
| // Disabled a cluster for a namespace manually. | ||
| if (nsPolicies.isPresent() && !isCurrentClusterAllowed(topicName.getNamespaceObject(), nsPolicies.get())) { |
There was a problem hiding this comment.
Possible regression: isCurrentClusterAllowed(NamespaceName, Policies) returns false when namespace replication_clusters excludes the current cluster (with empty allowed_clusters). But topic-level replicationClusters is supposed to override namespace-level defaults — e.g. ns replication_clusters=[c1], topic replicationClusters=[c1,c2] should be allowed on c2, but this short-circuit returns false.
Suggest only short-circuiting on the allowed_clusters hard gate (PIP-321), then deferring to topic-level checks before falling back to ns replication_clusters.
There was a problem hiding this comment.
Possible regression: isCurrentClusterAllowed(NamespaceName, Policies) returns false when namespace replication_clusters excludes the current cluster (with empty allowed_clusters). But topic-level replicationClusters is supposed to override namespace-level defaults — e.g. ns replication_clusters=[c1], topic replicationClusters=[c1,c2] should be allowed on c2, but this short-circuit returns false.
This situation does not exist. Let's review all possible use cases:
- Both clusters use a shared metadata store
a. The topic will never be allowed to be accessed by the current cluster, even if the topic-level replication policy contains{current cluster}.
b. The PIP-321 Introduceallowed-clusterat the namespace level introduces conceptnamespace level allowed_clustersto let clusters that using shared metadata store can enable topic level replication. - Both clusters use their own metadata store
a. All namespace-levelreplication_clutserswill contain the current cluster, and namespace-levelreplication clustercan not be set to a empty value.
b. No one will removecurrent clusterfrom namespace-levelreplication_clusters, which is meaningless
poorbarcode
dismissed
Denovo1998’s stale review
I have ansered him for a long time, he did not reply
poorbarcode
force-pushed
the
fix/create_missing_partitions_without_allowed_clusters
branch
from
86d621f to
8d9cabc
Compare
4 participants
Motivation
Env:
cluster-1andcluster-2share configuration metadata storepublic/default/tp1/admin/partitioned-topics/{topic}will be created on the shared configuration metadata store.cluster-1andcluster-2cluster-2from the topic-levelreplication policiespublic/default/tp1-partition-0will be deleted automatically.Issue: call
pulsar-admin topics create-missed-partitions <topic>, the partitionpublic/default/tp1-partition-0was loaded up again.Modifications
Fix the issue
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: x