Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Fix inconsistent topic policy #21231

Merged
merged 31 commits into from
Sep 26, 2023
Merged

[fix][broker] Fix inconsistent topic policy #21231

merged 31 commits into from
Sep 26, 2023

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 coderzc, 315157973, hangc0276, Technoboy- and codelipenghui 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.
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