You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2025/11/05 11:36:31 UTC
[PR] [branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
BewareMyPower opened a new pull request, #24947:
URL: https://github.com/apache/pulsar/pull/24947
### Motivation
The cherry-picking on a399b5ccd317282e8624135e49e08843c7196887 inserts the topic future cache duplicatedly.
1st time:
https://github.com/apache/pulsar/blob/74931c9e5f2f4384ed3b048101051dd8e5ea4a9a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1129
or
https://github.com/apache/pulsar/blob/74931c9e5f2f4384ed3b048101051dd8e5ea4a9a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1141
2nd time: https://github.com/apache/pulsar/blob/74931c9e5f2f4384ed3b048101051dd8e5ea4a9a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1146
In this case, topic future might fail with timeout. This error was exposed after https://github.com/apache/pulsar/commit/dd149d4ba4b9bc59bbc793fb083abe09252be5bf because the future completed with `LowOverheadTimeoutException` won't trigger the cache invalidation.
### Modifications
Move the `fetchPartitionedTopicMetadataAsync` check to `loadOrCreatePersistentTopic`.
### 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 -->
### Matching PR in forked repository
PR in forked repository:
--
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][branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24947:
URL: https://github.com/apache/pulsar/pull/24947#issuecomment-3490892390
> Well, there has been a debate before in https://github.com/apache/pulsar/pull/24500/files#r2199584548
>
> The timeout failure usually represents a unknown state. Removing it from the cache cannot cancel the ongoing topic loading process in background. There could be two concurrent topic loading tasks if we added the logic, some tests might fail (you can see the changes in #24500). We have to take care of this case.
>
> Topic loading for non-persistent topics is much more simple than persistent topics, so actually the timeout exception hardly happen.
@BewareMyPower Yes, I agree. I came to the same conclusion when looking into the code. However, there's also problems in the current CompletableFuture & timeout based solution. For example, if the topic has already opened the ledger when the timeout occurs, it won't be able to complete the future successfully. The topic needs to be closed after it just got successfully opened:
https://github.com/apache/pulsar/blob/39bb67542f2a7b849acaff681d408c693e1a2a18/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1896-L1914
This doesn't make much sense. Instead of using CompletableFuture values in maps, a better approach would be to have some Entry class that holds more related state. There could be a CompletableFuture inside the Entry class.
In the topic loading timeout scenario, it doesn't make sense to terminate the topic loading if it's already opening the ledger.
The system would just get more loaded due to closing the topic again and reopening it in a retry.
--
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][branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24947:
URL: https://github.com/apache/pulsar/pull/24947#issuecomment-3490759926
/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][branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24947:
URL: https://github.com/apache/pulsar/pull/24947#issuecomment-3490787340
> In this case, topic future might fail with timeout. This error was exposed after [dd149d4](https://github.com/apache/pulsar/commit/dd149d4ba4b9bc59bbc793fb083abe09252be5bf) because the future completed with `LowOverheadTimeoutException` won't trigger the cache invalidation.
Just wondering if there should be code to handle this. Similar as in `createNonPersistentTopic`
https://github.com/apache/pulsar/blob/74931c9e5f2f4384ed3b048101051dd8e5ea4a9a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1389-L1395
--
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][branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24947:
URL: https://github.com/apache/pulsar/pull/24947#issuecomment-3490853238
Well, there has been a debate before in https://github.com/apache/pulsar/pull/24500/files#r2199584548
The timeout failure usually represents a unknown state. Removing it from the cache cannot cancel the ongoing topic loading process in background. There could be two concurrent topic loading tasks if we added the logic, some tests might fail (you can see the changes in #24500). We have to take care of this case.
Topic loading for non-persistent topics is much more simple than persistent topics, so actually the timeout exception hardly happen.
--
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][branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24947:
URL: https://github.com/apache/pulsar/pull/24947#issuecomment-3490967410
IMO, we should not apply a timeout on topic loading. When the timeout happens, there must be a bug or the CPU resource is exhausted. You can see `topicLoadTimeoutSeconds` is 60, which is already too long. Just imagine creating a producer and the future does not complete in 60 seconds, or a reconnection due to topic unloading didn't succeed in 60 seconds.
In production env, the timeout is usually caused by topic replay because the metadata store operation should not be such slow:
1. `__change_events`
2. or message deduplication replay on the topic
The 1st case is usually caused by a bug with system topic based topic policies service. It is hard to say whether retrying helps.
The 2nd case is usually caused by the snapshot was not taken in time. Retry the topic replay never helps for the 2nd case. That's also the motivation of https://github.com/apache/pulsar/pull/23004, which introduces `testFinishTakeSnapshotWhenTopicLoading`.
I've been working on this part before but I didn't have time recently. Here are some local notes I wrote before.
----
Specifically, for a partition of a partitioned persistent topic, the topic load process is as follows:
1. Check if the topic exists (Metadata store: `/managed-ledgers/<persistence-naming-encoding>`)
2. Get topic policies (via `TopicPoliciesService#getTopicPoliciesAsync`)
3. Check topic ownership
4. Check if the pending topic loading count exceeds the `maxConcurrentTopicLoadRequest` config (default: 5000), if not, the following steps will be executed after the previous topic loading operations are completed.
5. Check topic ownership again
6. Fetch topic properties (Metadata store: `/admin/partitioned-topics/<tenant>/<namespace>/persistent/<encoded-topic>`)
7. Check if the topic count in the namespace will exceed the `maxTopicsPerNamespace` namespace policy (default: 1000) (Two metadata store operations, one is same with the 1st step, the other is `/admin/policies/<tenant>/<namespace>`)
8. Check if the cluster migration is enabled (Metadata store: `/admin/clusters/<cluster>/policies` and `/admin/local-policies/<tenant>/<namespace>`)
9. Validate topic partition metadata consistency (see PIP-414), which is similar to step 6
10. Get managed ledger config, it will require the topic policies (same with step 2) and namespace policies (Metadata store: `/admin/policies/<tenant>/<namespace>`)
11. Create the managed ledegr via `ManagedLedgerFactory#asyncOpen`
12. Create the `PersistentTopic` instance via `TopicFactory#create` and perform some initializations
Normally, the metadata store operations above are fast because many operations are done in previous RPC during topic lookup and the results will be cached. The main concern is the topic level policies, whose default implementation is to read from the `__change_events` topic in the same namespace with the user topic.
Apart from the topic policies, each topic has its unique properties stored in metadata store as well:
- Partitioned topic metadata: `/admin/partitioned-topics/<tenant>/<namespace>/<topic>`
- Managed ledger properties: `/managed-ledgers/<persistence-naming-encoding>` (it's actually partition level)
Topic policies are good to manage massive topic level metadata without touching the metadata store too much. Each topic will register itself as a listener to `TopicPoliciesService` so it can be aware of the topic policies changes. However, block
ing the topic loading process to wait for the topic policies is not a good idea, especially for the case when the namespace was not owned by the new owner broker before. When there are many namespaces, each namespace will create a reader to repl
ay the whole `<tenant>/<ns>/__change_events` topic, which could bring much pressure on BookKeeper and then "too many xxx requests" could happen.
The topic policies are used to apply the `PersistencePolicies`, `RetentionPolicies` and `OffloadPolicies` on the managed ledger config. However, these policies also exist at namespace level.
--
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][branch-4.0] Fix failed testFinishTakeSnapshotWhenTopicLoading due to topic future cache conflicts [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #24947:
URL: https://github.com/apache/pulsar/pull/24947
--
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