Skip to content

Conversation

@mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Sep 21, 2023

Motivation

#7817 Introduced topic-level policy for a managed-config with a bug. Which can lead to an unexpected data retention policy If the pulsar topic policy service isn't initialized.

Modifications

  • Introduced some new pure async methods
    • CompletableFuture<Optional<TopicPolicies>> getTopicPoliciesAsync(@Nonnull TopicName topicName, boolean isGlobal);
    • CompletableFuture<Optional<TopicPolicies>> getTopicPoliciesAsync(@Nonnull TopicName topicName);
  • Make BrokerService use pure async method.

Verifying this change

  • Make sure that the change passes the CI checks.

Alternatives

  • Support lazy get retention-related managed ledger configuration.
  • Use TopicPoliciesService#CompletableFuture<Optional<TopicPolicies>> getTopicPoliciesAsyncWithRetry(TopicName topicName, final Backoff backoff, ScheduledExecutorService scheduledExecutorService, boolean isGlobal) method.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@mattisonchao mattisonchao added type/bug The PR fixed a bug or issue reported a bug area/broker category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost labels Sep 21, 2023
@mattisonchao mattisonchao self-assigned this Sep 21, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 21, 2023
@mattisonchao mattisonchao added this to the 3.2.0 milestone Sep 21, 2023
@mattisonchao mattisonchao marked this pull request as draft September 22, 2023 04:01
@mattisonchao
Copy link
Member Author

I need do some changes for fixing performance issue.

@mattisonchao mattisonchao marked this pull request as ready for review September 22, 2023 07:47
@mattisonchao mattisonchao reopened this Sep 22, 2023
@coderzc
Copy link
Member

coderzc commented Sep 22, 2023

Since getTopicPoliciesAsync has been introduced, should we consider using getTopicPoliciesAsync instead of getTopicPoliciesAsyncWithRetry and getTopicPolicies?

@mattisonchao
Copy link
Member Author

@coderzc Yep, you are right. I will do it in another PR.

@mattisonchao mattisonchao requested review from 315157973, Technoboy-, codelipenghui, coderzc and hangc0276 and removed request for coderzc September 22, 2023 08:56
@asafm
Copy link
Contributor

asafm commented Sep 26, 2023

The methods before were not async?

@mattisonchao
Copy link
Member Author

The methods before were not async?

Yes, it was used backoff to implement an async-like approach.

liangyuanpeng pushed a commit to liangyuanpeng/pulsar that referenced this pull request Oct 11, 2023
codelipenghui pushed a commit that referenced this pull request Nov 8, 2023
### Motivation

PR #21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`.

It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout.


### Modifications

- Get the topic policy before loading.
Technoboy- pushed a commit that referenced this pull request Nov 10, 2023
PR #21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`.

It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout.

- Get the topic policy before loading.
nborisov pushed a commit to nborisov/pulsar that referenced this pull request Nov 13, 2023
…21445)

### Motivation

PR apache#21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`.

It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout.


### Modifications

- Get the topic policy before loading.
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
…21445)

PR apache#21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`.

It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout.

- Get the topic policy before loading.
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
…21445)

PR apache#21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`.

It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout.

- Get the topic policy before loading.
bahetimansi pushed a commit to bahetimansi/pulsar that referenced this pull request Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 cherry-picked/branch-3.1 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.6 release/2.11.3 release/3.0.2 release/3.1.2 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants