Skip to content

[fix][broker] Fix the applying of namespace policies - #22504

Closed
nodece wants to merge 3 commits into
apache:masterfrom
nodece:fix-replicator-rate-limit-loading
Closed

[fix][broker] Fix the applying of namespace policies#22504
nodece wants to merge 3 commits into
apache:masterfrom
nodece:fix-replicator-rate-limit-loading

Conversation

@nodece

@nodece nodece commented Apr 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Motivation

I'm using the rate limiter on the replicator, when an existing topic is initializing, and the replicator's cursor exists, the topic will create the replicator, and then load and apply the namespace and topic policies to the topic(which includes the replicator), but I found the rate limiter doesn't work.

The namespace policies will not be applied to the replicator after the broker restart.

When will this happen:

  • The topic policies are disabled.
  • The topic policies are enabled, and there are no policies.

Modifications

  • Loading the compaction service and replicator after the namespace and topic policies are loaded
  • When there are no topic policies, apply the namespace policies to the topic
  • Add resourceGroup, encryptionRequired, and allowAutoUpdateSchema to the HierarchyTopicPolicies to unify the policies

Documentation

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

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 15, 2024
@nodece
nodece marked this pull request as draft April 15, 2024 08:07
@nodece
nodece force-pushed the fix-replicator-rate-limit-loading branch from 64e83cf to 7bc6d44 Compare April 15, 2024 08:47
@nodece nodece changed the title [fix][broker] Fix the rate limit policy loading for replicator after broker restart [fix][broker] Apply namespace policy to topic after broker restart Apr 15, 2024
@nodece
nodece marked this pull request as ready for review April 15, 2024 09:12
@nodece
nodece force-pushed the fix-replicator-rate-limit-loading branch from 7bc6d44 to d9fc1c1 Compare April 15, 2024 09:27
@nodece nodece changed the title [fix][broker] Apply namespace policy to topic after broker restart [fix][broker] Fix the applying of namespace policies Apr 15, 2024
nodece
nodece commented Apr 15, 2024
View reviewed changes
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
}

protected CompletableFuture<Void> initTopicPolicy() {
protected CompletableFuture<Void> initTopicPolicyAndApply() {

@nodece nodece Apr 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call thread is getOrderedExecutor().

@nodece
nodece force-pushed the fix-replicator-rate-limit-loading branch 2 times, most recently from 43bd4e2 to 131af38 Compare April 15, 2024 10:35
@liudezhi2098

liudezhi2098 commented Apr 16, 2024

Copy link
Copy Markdown
Contributor

Check CI errors first

@nodece

nodece commented Apr 16, 2024

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@nodece
nodece marked this pull request as draft April 16, 2024 05:56
@nodece
nodece force-pushed the fix-replicator-rate-limit-loading branch from 8bf364b to 131af38 Compare April 16, 2024 10:08
@nodece
nodece marked this pull request as ready for review April 16, 2024 10:17
nodece
nodece commented Apr 16, 2024
View reviewed changes
Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java Outdated
recycle();
});
} else if (cursor.config.isAutoSkipNonRecoverableData() && exception instanceof NonRecoverableLedgerException) {
} else if (cursor.getManagedLedger().getConfig().isAutoSkipNonRecoverableData()

@nodece nodece Apr 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cursor.config equals getManagedLedger().getConfig().

When the topic is loaded, org.apache.pulsar.broker.service.persistent.PersistentTopic#checkPersistencePolicies will be called, which changes the ledger config, but the cursor.config will not be changed, so we need to fix that to get the latest config.

BTW, the cursor.conf should be deleted, and we should always call the getManagedLedger().getConfig().

@nodece

nodece commented Apr 16, 2024

Copy link
Copy Markdown
Member Author

@liudezhi2098 The CI has been passed.

@nodece
nodece force-pushed the fix-replicator-rate-limit-loading branch from fcbf307 to 3aaa7d5 Compare May 10, 2024 15:43
@nodece nodece mentioned this pull request Jun 12, 2024
Merged
1 of 4 tasks
@nodece
nodece marked this pull request as draft June 13, 2024 01:45
@nodece

nodece commented Jun 28, 2024

Copy link
Copy Markdown
Member Author

Closed by #22890

@nodece nodece closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari Awaiting requested review from lhotari
@Technoboy- Technoboy- Awaiting requested review from Technoboy-
@poorbarcode poorbarcode Awaiting requested review from poorbarcode
@liudezhi2098 liudezhi2098 Awaiting requested review from liudezhi2098

Assignees

@nodece nodece

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nodece @liudezhi2098 @lhotari