You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/29 00:39:05 UTC
[GitHub] [pulsar] aymkhalil opened a new pull request, #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
aymkhalil opened a new pull request, #19097:
URL: https://github.com/apache/pulsar/pull/19097
Continuation of #18518 and #17609 to ensure the ns policy `deleted` is in consistent state between brokers when performing and `isAllowAutoTopicCreationAsync`
--
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
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#discussion_r1058961225
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -3157,42 +3157,38 @@ public CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final String top
TopicName topicName = TopicName.get(topic);
return isAllowAutoTopicCreationAsync(topicName);
}
-
public CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName) {
- Optional<Policies> policies =
- pulsar.getPulsarResources().getNamespaceResources()
- .getPoliciesIfCached(topicName.getNamespaceObject());
- return isAllowAutoTopicCreationAsync(topicName, policies);
- }
-
- private CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName,
- final Optional<Policies> policies) {
- if (policies.isPresent() && policies.get().deleted) {
- log.info("Preventing AutoTopicCreation on a namespace that is being deleted {}",
- topicName.getNamespaceObject());
- return CompletableFuture.completedFuture(false);
- }
- //System topic can always be created automatically
- if (pulsar.getConfiguration().isSystemTopicEnabled() && isSystemTopic(topicName)) {
- return CompletableFuture.completedFuture(true);
- }
- final boolean allowed;
- AutoTopicCreationOverride autoTopicCreationOverride = getAutoTopicCreationOverride(topicName, policies);
- if (autoTopicCreationOverride != null) {
- allowed = autoTopicCreationOverride.isAllowAutoTopicCreation();
- } else {
- allowed = pulsar.getConfiguration().isAllowAutoTopicCreation();
- }
-
- if (allowed && topicName.isPartitioned()) {
- // cannot re-create topic while it is being deleted
- return pulsar.getPulsarResources().getNamespaceResources().getPartitionedTopicResources()
- .isPartitionedTopicBeingDeletedAsync(topicName)
- .thenApply(beingDeleted -> !beingDeleted);
- } else {
- return CompletableFuture.completedFuture(allowed);
- }
-
+ return pulsar.getPulsarResources().getNamespaceResources()
+ .getPoliciesAsync(topicName.getNamespaceObject(), true)
Review Comment:
+1
--
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
[GitHub] [pulsar] aymkhalil commented on a diff in pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by GitBox <gi...@apache.org>.
aymkhalil commented on code in PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#discussion_r1059203687
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -3157,42 +3157,38 @@ public CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final String top
TopicName topicName = TopicName.get(topic);
return isAllowAutoTopicCreationAsync(topicName);
}
-
public CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName) {
- Optional<Policies> policies =
- pulsar.getPulsarResources().getNamespaceResources()
- .getPoliciesIfCached(topicName.getNamespaceObject());
- return isAllowAutoTopicCreationAsync(topicName, policies);
- }
-
- private CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName,
- final Optional<Policies> policies) {
- if (policies.isPresent() && policies.get().deleted) {
- log.info("Preventing AutoTopicCreation on a namespace that is being deleted {}",
- topicName.getNamespaceObject());
- return CompletableFuture.completedFuture(false);
- }
- //System topic can always be created automatically
- if (pulsar.getConfiguration().isSystemTopicEnabled() && isSystemTopic(topicName)) {
- return CompletableFuture.completedFuture(true);
- }
- final boolean allowed;
- AutoTopicCreationOverride autoTopicCreationOverride = getAutoTopicCreationOverride(topicName, policies);
- if (autoTopicCreationOverride != null) {
- allowed = autoTopicCreationOverride.isAllowAutoTopicCreation();
- } else {
- allowed = pulsar.getConfiguration().isAllowAutoTopicCreation();
- }
-
- if (allowed && topicName.isPartitioned()) {
- // cannot re-create topic while it is being deleted
- return pulsar.getPulsarResources().getNamespaceResources().getPartitionedTopicResources()
- .isPartitionedTopicBeingDeletedAsync(topicName)
- .thenApply(beingDeleted -> !beingDeleted);
- } else {
- return CompletableFuture.completedFuture(allowed);
- }
-
+ return pulsar.getPulsarResources().getNamespaceResources()
+ .getPoliciesAsync(topicName.getNamespaceObject(), true)
Review Comment:
I traced http vs binary protocol calls that invoke this code:
http calls:
- resetCursorOnPosition
- lookupTopic
- createSubscription
- grantPermissionsOnTopic
- getPartitionedMetadata
- Basically any method calling the [AdminResource#getPartitionedTopicMetadataAsync](https://github.com/apache/pulsar/blob/416c5b40ac1c44bbe281dba35ea6682dae7697da/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L482) with `checkAllowAutoCreation` true
binary protocol calls:
- handlePartitionMetadataRequest
- handleSubscribe
- handleProducer
I isolated the calls with as minimum code changes as possible. Please evaluate the value of the change vs the introduced code branches.
PTAL.
--
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
[GitHub] [pulsar] github-actions[bot] commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1367001432
@aymkhalil Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `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
[GitHub] [pulsar] eolivelli commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1368198250
At first glance now the patch looks good, but I will review it more carefully next week
--
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
[GitHub] [pulsar] github-actions[bot] commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by github-actions.
github-actions[bot] commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1409632951
The pr had no activity for 30 days, mark with Stale label.
--
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
[GitHub] [pulsar] aymkhalil commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by "aymkhalil (via GitHub)" <gi...@apache.org>.
aymkhalil commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1411025708
> Have you considered the double-check:
use cached policies to decide on eligibility as without your changes, check again with refresh right before managedLedgerFactory.asyncOpen call in the BrokerService, fail if ns.deleted.
I didn't explicitly, the approach was to trace all http calls that touches the ns policy ([here](https://github.com/apache/pulsar/pull/19097#discussion_r1059203687)) so we cover more cases, but not cases where this could add unnecessary overhead (hence the distinction between http vs binary calls).
But you made me think, actually the automatic topic creation triggered by a producer is actually on the binary protocol path so I either have to make all `isAllowAutoTopicCreationAsync` use the refresh API, or consider limiting the scope to the ML open async as you proposed...
--
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
[GitHub] [pulsar] eolivelli commented on a diff in pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#discussion_r1058811705
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -3157,42 +3157,38 @@ public CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final String top
TopicName topicName = TopicName.get(topic);
return isAllowAutoTopicCreationAsync(topicName);
}
-
public CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName) {
- Optional<Policies> policies =
- pulsar.getPulsarResources().getNamespaceResources()
- .getPoliciesIfCached(topicName.getNamespaceObject());
- return isAllowAutoTopicCreationAsync(topicName, policies);
- }
-
- private CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName,
- final Optional<Policies> policies) {
- if (policies.isPresent() && policies.get().deleted) {
- log.info("Preventing AutoTopicCreation on a namespace that is being deleted {}",
- topicName.getNamespaceObject());
- return CompletableFuture.completedFuture(false);
- }
- //System topic can always be created automatically
- if (pulsar.getConfiguration().isSystemTopicEnabled() && isSystemTopic(topicName)) {
- return CompletableFuture.completedFuture(true);
- }
- final boolean allowed;
- AutoTopicCreationOverride autoTopicCreationOverride = getAutoTopicCreationOverride(topicName, policies);
- if (autoTopicCreationOverride != null) {
- allowed = autoTopicCreationOverride.isAllowAutoTopicCreation();
- } else {
- allowed = pulsar.getConfiguration().isAllowAutoTopicCreation();
- }
-
- if (allowed && topicName.isPartitioned()) {
- // cannot re-create topic while it is being deleted
- return pulsar.getPulsarResources().getNamespaceResources().getPartitionedTopicResources()
- .isPartitionedTopicBeingDeletedAsync(topicName)
- .thenApply(beingDeleted -> !beingDeleted);
- } else {
- return CompletableFuture.completedFuture(allowed);
- }
-
+ return pulsar.getPulsarResources().getNamespaceResources()
+ .getPoliciesAsync(topicName.getNamespaceObject(), true)
Review Comment:
I think that there maybe some case in which it is good to use the local version of the Policies object.
We need to use "refresh" only when we reach here from a HTTP API call,
I know that is looks pretty convoluted, but with this change in the current form we are going to use "refresh" (that is a heavyweight operation) probably in code paths that don't need it
--
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
[GitHub] [pulsar] eolivelli closed pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli closed pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
URL: https://github.com/apache/pulsar/pull/19097
--
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
[GitHub] [pulsar] aymkhalil commented on a diff in pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by GitBox <gi...@apache.org>.
aymkhalil commented on code in PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#discussion_r1059203793
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -4217,62 +4215,6 @@ protected void internalOffloadStatus(AsyncResponse asyncResponse, boolean author
});
}
- public static CompletableFuture<PartitionedTopicMetadata> getPartitionedTopicMetadata(
Review Comment:
this is not used anywhere, removed it to minimize the code paths we need to think about.
--
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
[GitHub] [pulsar] dlg99 commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1410908639
Have you considered the double-check:
use cached policies to decide on eligibility as without your changes, check again with refresh right before `managedLedgerFactory.asyncOpen` call in the BrokerService, fail if ns.deleted.
--
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
[GitHub] [pulsar] aymkhalil commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by "aymkhalil (via GitHub)" <gi...@apache.org>.
aymkhalil commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1410930325
> FWIW I don't think this change alone can completely prevent the topic creation during ns deletion: one can sync the policy and get the latest one, then ns deletion starts, then we create a topic - there is no locking/distributed locking for these operations. It might reduce frequency of the problem occurrence.
+1. If fact, any "read" based solution to achieve consistency will be best effort only. I don't see how it could be solved for good without conditional writes which are not well supported. For example, in ZK the setData API does not support something like "fail if any version exists" but also there will be need to coordinate updates because the metadata for policy and topic (managed ledger) are under different nodes.
So, I couldn't find an easy way to do it that fits in the scope/purpose of this patch...
--
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
[GitHub] [pulsar] aymkhalil commented on pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by "aymkhalil (via GitHub)" <gi...@apache.org>.
aymkhalil commented on PR #19097:
URL: https://github.com/apache/pulsar/pull/19097#issuecomment-1419505538
After collecting feedback, I'm closing this PR for now bc:
1. It does not address the consistency problem for auto created topics since those happen on the binary protocol path (i.e. a producer)
2. Concerns about extra overhead of calling the `sync` API on multiple code paths
3. Even if we limit the `sync` API to when ML.asyncOpen() call is attempted, it means that for 100% of topic creation attempt we enforce the sync although the problem happens only in corner cases. The actual impact here is not measured due to lack of data points regarding the `sync` API latency && the frequency of the delete problem (clarified in the next point).
4. The underlying issue (which is creating topics while ns delete is in progress) could be already mitigated to an extent with the delete retries implemented in #19374
So the idea would be to:
1. Monitor if the problem happens again after merging #19374
2. Reproduce the problem
3. Submit another PR for the fix in-light of the above information (please note that all solutions are best effort only as explained here: https://github.com/apache/pulsar/pull/19097#issuecomment-1410930325
--
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
[GitHub] [pulsar] aymkhalil closed pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
Posted by "aymkhalil (via GitHub)" <gi...@apache.org>.
aymkhalil closed pull request #19097: [improve] Refresh ns policy when deciding auto topic creation eligibility
URL: https://github.com/apache/pulsar/pull/19097
--
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