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/04/28 07:00:08 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
poorbarcode opened a new pull request, #15363:
URL: https://github.com/apache/pulsar/pull/15363
### Motivation
When execute command `ulsar-admin topicPolicies get-deduplication persistent://{tenant}/{ns}/{topic}` failure.
Then the `deduplication task` will be disabled, and cursor 'pulsar.dedup' will not move still restart `broker`.
But zk-node: `/managed-ledgers/{tenant}/{ns}/persistent/{topic}/pulsar.dedup` still exists.
Errors occur: this topic's ledger-store will not be deleted anymore, even if all message has been acknowledged.
### Modifications
When `checkDeduplicationStatus()` failure, stop this topic.
### Documentation
Need to update docs?
- [ ] `doc-required`
- [x] `no-need-doc`
- [ ] `doc`
- [ ] `doc-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] github-actions[bot] commented on pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1146498004
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] mattisonchao commented on pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1111835563
Another problem:
Can we add tests to verify this issue?
--
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 pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1113928841
/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
[GitHub] [pulsar] poorbarcode commented on pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1111965992
> Little question: I'm not sure if the issue is unrecoverable, but it feels a little bad to close the topic. Please let me know what you think.
> I'm not sure if the issue is unrecoverable, but it feels a little bad to close the topic.
If `deduplication` can't work properly, means `topic` can't work properly.
When user changes `deduplication status`, it should be clear that works or not.
Do `close topic` will rerty `start deduplication task` or told user `topic was done`, both of these results are useful to user
--
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] Retry the dedup cursor delete after a failure while disabling at topic load [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-6370101981
Hi @poorbarcode, thanks for this, and sorry it sat so long.
Since 2022 master has caught up with most of what this PR fixed: #25982 now lets a failed disable go back through the `Failed` status and retry the cursor delete. There's one case it still misses. When a topic loads with dedup already disabled and the `pulsar.dedup` cursor delete fails with anything other than `CursorNotFoundException`, the status stays at `Removing`. The `Removing` branch only reschedules `checkStatus()`, so the delete is never retried, and the cursor keeps ledgers from being deleted until the topic is unloaded.
I've taken over the branch to fix just that case. I merged master in (no force-push), dropped the earlier state-machine rewrite, and changed that failure path to set `Failed` so the next `checkStatus()` retries the delete. I also added a test in `BrokerMessageDeduplicationTest`, and updated the title and description to match. I left out the blocking `checkDeduplicationStatus().get()` in `onUpdate` and closing the topic on failure, because a retry covers the same case without closing a live topic.
Please take a look when you have a moment and let me know if I missed anything from your original case.
--
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 #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1258878045
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] congbobo184 commented on pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1318538778
@poorbarcode hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.
--
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] michaeljmarshall commented on pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1610272184
As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 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
Re: [PR] [fix][broker] Retry the dedup cursor delete after a failure while disabling at topic load [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #15363:
URL: https://github.com/apache/pulsar/pull/15363
--
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 #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#discussion_r860667133
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3060,7 +3060,21 @@ public void onUpdate(TopicPolicies policies) {
checkReplicationAndRetryOnFailure();
}
- checkDeduplicationStatus();
+ /**
+ * When execute command "pulsar-admin topicPolicies get-deduplication persistent://{tenant}/{ns}/{topic}"
+ * but "store.asyncRemoveCursor('pulsar.dedup')" failure, the 'pulsar-dedup' cursor will be disabled,
+ * zk-node: "/managed-ledgers/{tenant}/{ns}/persistent/{topic}/pulsar.dedup" still exists.
+ * Then this topic's ledger-store will not be deleted anymore, even if all message has been acknowledged.
+ * So when `checkDeduplicationStatus()` failure, stop this topic.
+ */
+ try {
+ checkDeduplicationStatus().get();
Review Comment:
Why ? I tried it , it works.
--
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] mattisonchao commented on a diff in pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#discussion_r860556076
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3060,7 +3060,21 @@ public void onUpdate(TopicPolicies policies) {
checkReplicationAndRetryOnFailure();
}
- checkDeduplicationStatus();
+ /**
+ * When execute command "pulsar-admin topicPolicies get-deduplication persistent://{tenant}/{ns}/{topic}"
+ * but "store.asyncRemoveCursor('pulsar.dedup')" failure, the 'pulsar-dedup' cursor will be disabled,
+ * zk-node: "/managed-ledgers/{tenant}/{ns}/persistent/{topic}/pulsar.dedup" still exists.
+ * Then this topic's ledger-store will not be deleted anymore, even if all message has been acknowledged.
+ * So when `checkDeduplicationStatus()` failure, stop this topic.
+ */
+ try {
+ checkDeduplicationStatus().get();
Review Comment:
At first glance, we cannot call sync method inside async method.
--
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 #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1170787028
@poorbarcode Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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 #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1170785572
@poorbarcode Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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] mattisonchao commented on a diff in pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#discussion_r860556076
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3060,7 +3060,21 @@ public void onUpdate(TopicPolicies policies) {
checkReplicationAndRetryOnFailure();
}
- checkDeduplicationStatus();
+ /**
+ * When execute command "pulsar-admin topicPolicies get-deduplication persistent://{tenant}/{ns}/{topic}"
+ * but "store.asyncRemoveCursor('pulsar.dedup')" failure, the 'pulsar-dedup' cursor will be disabled,
+ * zk-node: "/managed-ledgers/{tenant}/{ns}/persistent/{topic}/pulsar.dedup" still exists.
+ * Then this topic's ledger-store will not be deleted anymore, even if all message has been acknowledged.
+ * So when `checkDeduplicationStatus()` failure, stop this topic.
+ */
+ try {
+ checkDeduplicationStatus().get();
Review Comment:
At first glance, we cannot call the sync method inside the async method.
--
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 pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1111959261
> Little question: I'm not sure if the issue is unrecoverable, but it feels a little bad to close the topic. Please let me know what you think.
> Another question: Can we add tests to verify this issue?
OK. Thank you for reminding me. liang zai
--
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 #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1170786066
@poorbarcode Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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 pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1154268113
/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
[GitHub] [pulsar] poorbarcode commented on pull request #15363: [fix] [broker] Disabled deduplication, but ledger-store will not be d…
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15363:
URL: https://github.com/apache/pulsar/pull/15363#issuecomment-1153856061
/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