You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nodece (via GitHub)" <gi...@apache.org> on 2024/04/15 05:41:04 UTC
[PR] [fix][broker] Fix the rate limit policy loading for replicator after broker restart [pulsar]
nodece opened a new pull request, #22504:
URL: https://github.com/apache/pulsar/pull/22504
### Motivation
Initializing a topic, if a replicator's cursor exists, the topic will create the replicator, and then load the namespace and topic policy, when the policies are loaded, we should apply that to the replicator.
The namespace policies will not be applied to the replicator after the broker restart when topic policies are empty, which causes an excess of the total traffic.
### Modifications
- Call `org.apache.pulsar.broker.service.persistent.PersistentTopic#onPoliciesUpdate` after the namespace policy is loaded
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#discussion_r1565494207
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -4025,16 +4028,19 @@ private void updateSubscriptionsDispatcherRateLimiter() {
});
}
- protected CompletableFuture<Void> initTopicPolicy() {
+ protected CompletableFuture<Void> initTopicPolicyAndApply() {
Review Comment:
This call thread is `brokerService.getTopicOrderedExecutor()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#issuecomment-2058147664
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#discussion_r1567121175
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -111,7 +111,8 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
callback.readEntriesComplete(entries, ctx);
recycle();
});
- } else if (cursor.config.isAutoSkipNonRecoverableData() && exception instanceof NonRecoverableLedgerException) {
+ } else if (cursor.getManagedLedger().getConfig().isAutoSkipNonRecoverableData()
Review Comment:
`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 always call the `getManagedLedger().getConfig()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#discussion_r1567121175
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -111,7 +111,8 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
callback.readEntriesComplete(entries, ctx);
recycle();
});
- } else if (cursor.config.isAutoSkipNonRecoverableData() && exception instanceof NonRecoverableLedgerException) {
+ } else if (cursor.getManagedLedger().getConfig().isAutoSkipNonRecoverableData()
Review Comment:
`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()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "liudezhi2098 (via GitHub)" <gi...@apache.org>.
liudezhi2098 commented on PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#issuecomment-2058054390
Check CI errors first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#discussion_r1565494207
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -4025,16 +4028,19 @@ private void updateSubscriptionsDispatcherRateLimiter() {
});
}
- protected CompletableFuture<Void> initTopicPolicy() {
+ protected CompletableFuture<Void> initTopicPolicyAndApply() {
Review Comment:
This call thread is `getOrderedExecutor()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#issuecomment-2058160022
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] Fix the applying of namespace policies [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22504:
URL: https://github.com/apache/pulsar/pull/22504#issuecomment-2059157205
@liudezhi2098 The CI has been passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org