You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "sandeep-mst (via GitHub)" <gi...@apache.org> on 2025/08/18 07:21:56 UTC
[PR] [improve][broker] Add guard to prevent concurrent snapshot creation with warning log by ReplicationSubscriptionController [pulsar]
sandeep-mst opened a new pull request, #24636:
URL: https://github.com/apache/pulsar/pull/24636
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #24635
<!-- or this PR is one task of an issue -->
<!-- If the PR belongs to a PIP, please add the PIP link here -->
<!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/pip/README.md -->
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
Replicated subscription snapshot creation is scheduled at a fixed interval. There is a risk of initiating overlapping snapshot tasks in slow environments with small interval.
While `lastCompletedSnapshotStartTime` already prevents redundant snapshot creation when no new data is present, it does not safeguard against concurrent execution if a previous snapshot is still in progress.
### Modifications
<!-- Describe the modifications you've done. -->
- Introduced an AtomicBoolean `isRunning` to guard the `startNewSnapshot()` method and prevent concurrent executions.
- Added a warning log when a new snapshot is skipped due to one already in progress.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change added tests and can be verified as follows:
- *Will be updated*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### 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: <!-- ENTER URL HERE -->
https://github.com/cognitree/pulsar/pull/26
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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] [improve][broker] Add guard to prevent concurrent snapshot creation with warning log by ReplicationSubscriptionController [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24636:
URL: https://github.com/apache/pulsar/pull/24636#discussion_r2281527386
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/ReplicatedSubscriptionsController.java:
##########
@@ -92,6 +93,8 @@ public class ReplicatedSubscriptionsController implements AutoCloseable, Topic.P
private final OpenTelemetryReplicatedSubscriptionStats stats;
+ private final AtomicBoolean isRunning = new AtomicBoolean(false);
Review Comment:
`isRunning` is not a great name for this field. `startingNewSnapshot` would be better since this only applies to the `startNewSnapshot` method.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/ReplicatedSubscriptionsController.java:
##########
@@ -219,61 +222,71 @@ private void receiveSubscriptionUpdated(ReplicatedSubscriptionsUpdate update) {
}
private void startNewSnapshot() {
- cleanupTimedOutSnapshots();
-
- if (lastCompletedSnapshotStartTime == 0 && !pendingSnapshots.isEmpty()) {
- // 1. If the remote cluster has disabled subscription replication or there's an incorrect config,
- // it will not respond to SNAPSHOT_REQUEST. Therefore, lastCompletedSnapshotStartTime will remain 0,
- // making it unnecessary to resend the request.
- // 2. This approach prevents sending additional SNAPSHOT_REQUEST to both local_topic and remote_topic.
- // 3. Since it's uncertain when the remote cluster will enable subscription replication,
- // the timeout mechanism of pendingSnapshots is used to ensure retries.
- //
- // In other words, when hit this case, The frequency of sending SNAPSHOT_REQUEST
- // will use `replicatedSubscriptionsSnapshotTimeoutSeconds`.
- if (log.isDebugEnabled()) {
- log.debug("[{}] PendingSnapshot exists but has never succeeded. "
- + "Skipping snapshot creation until pending snapshot timeout.", topic.getName());
- }
+ if (!isRunning.compareAndSet(false, true)) {
+ log.warn("[{}] Snapshot already in progress. Skipping new snapshot.", topic.getName());
Review Comment:
`warn` level is perhaps too strong since this isn't a real problem. I think `info` would be more appropriate.
--
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] [improve][broker] Add guard to prevent concurrent snapshot creation with warning log by ReplicationSubscriptionController [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24636:
URL: https://github.com/apache/pulsar/pull/24636#issuecomment-3195481387
> Replicated subscription snapshot creation is scheduled at a fixed interval. There is a risk of initiating overlapping snapshot tasks in slow environments with small interval.
btw. a similar problem exists in multiple locations in Pulsar. There's #24596 to address such issues.
--
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] [improve][broker] Add guard to prevent concurrent snapshot creation with warning log by ReplicationSubscriptionController [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #24636:
URL: https://github.com/apache/pulsar/pull/24636#issuecomment-6347661600
Thanks for the PR, and sorry it sat so long. I don't think the guard can ever trigger. `startNewSnapshot()` is only called from the `scheduleAtFixedRate` timer on a `ScheduledThreadPoolExecutor`, which never runs the same task concurrently. The method body is also synchronous, since `builder.start()` only publishes the request marker. Several pending snapshots at once (each with its own ID) is expected. The `BadVersionException` in #24635 comes from a managed-ledger metadata update, not from the snapshot markers, so it likely has another cause, such as an ownership change or two writers on the cursor/ledger metadata. I'm closing this one. If you can still reproduce the BadVersion error, please attach the full stack trace and the broker logs around it to #24635 and we can look for the real cause there.
--
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] [improve][broker] Add guard to prevent concurrent snapshot creation with warning log by ReplicationSubscriptionController [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #24636: [improve][broker] Add guard to prevent concurrent snapshot creation with warning log by ReplicationSubscriptionController
URL: https://github.com/apache/pulsar/pull/24636
--
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