You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2024/09/27 09:15:05 UTC

[PR] [fix] [broker] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

poorbarcode opened a new pull request, #23360:
URL: https://github.com/apache/pulsar/pull/23360

   ### Motivation
   
   #### Background
   - Users may start 2 clusters with the Geo-Replication feature with Global ZK
   - Users want to hire one cluster, so remove the cluster from `namespace-level replicatedClusters`
     - Broker will remove the topics of the cluster removed automatically.
   - Issue occurs:
     - The system topic `__change_events` being deleted first.
     - User topics can not be deleted anymore, because the operation `delete topic-level policies` can not be executed successfully anymore.
     - The task `topic.checkReplication` and other operations will retry again and again, as a result, the broker will crash of OOM  
   
   ### Modifications
   - Skip removing topic-level policies if the system topic `__change_event` has been deleted.
   
   ### 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: x
   


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#discussion_r1793780487


##########
bin/ledgers.md:
##########


Review Comment:
   Removed, thanks for reminding this



-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2405414583

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23360?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 74.36%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`5c2e2b4`)](https://app.codecov.io/gh/apache/pulsar/commit/5c2e2b4bc7ebb20b9edef8d79fcb872e827cc819?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 660 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/23360/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/23360?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #23360      +/-   ##
   ============================================
   + Coverage     73.57%   74.36%   +0.78%     
   - Complexity    32624    34983    +2359     
   ============================================
     Files          1877     1953      +76     
     Lines        139502   147162    +7660     
     Branches      15299    16199     +900     
   ============================================
   + Hits         102638   109430    +6792     
   - Misses        28908    29305     +397     
   - Partials       7956     8427     +471     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23360/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23360/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.31% <62.50%> (+2.73%)` | :arrow_up: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23360/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.32% <62.50%> (-0.01%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23360/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.72% <100.00%> (+0.88%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23360?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [.../service/SystemTopicBasedTopicPoliciesService.java](https://app.codecov.io/gh/apache/pulsar/pull/23360?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FSystemTopicBasedTopicPoliciesService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N5c3RlbVRvcGljQmFzZWRUb3BpY1BvbGljaWVzU2VydmljZS5qYXZh) | `74.42% <100.00%> (+0.23%)` | :arrow_up: |
   | [...er/systopic/NamespaceEventsSystemTopicFactory.java](https://app.codecov.io/gh/apache/pulsar/pull/23360?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fsystopic%2FNamespaceEventsSystemTopicFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zeXN0b3BpYy9OYW1lc3BhY2VFdmVudHNTeXN0ZW1Ub3BpY0ZhY3RvcnkuamF2YQ==) | `100.00% <100.00%> (ø)` | |
   
   ... and [633 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23360/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   </details>


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2405193657

   Triggering CI with fix #23431 


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #23360:
URL: https://github.com/apache/pulsar/pull/23360


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2442117460

   @heesung-sn 
   
   > It appears that this PR fails the test in branch-3.3.
   
   Sure, it will be fixed by the PR: https://github.com/apache/pulsar/pull/23522


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2419401765

   > I wonder if this is causing https://github.com/apache/pulsar/issues/23474 which currently blocks Pulsar CI. There's PR https://github.com/apache/pulsar/pull/23478 to disable OneWayReplicatorTestBase tests the problem is addressed. @poorbarcode do you have a chance to fix the problem?
   
   Self-assigned https://github.com/apache/pulsar/issues/23474, I will fix it 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

Re: [PR] [fix] [broker] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2426746666

   @poorbarcode Please also handle cherry-picking to branch-3.0 and branch-3.3 


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#discussion_r1793782304


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -133,7 +133,13 @@ public CompletableFuture<Void> deleteTopicPoliciesAsync(TopicName topicName) {
         if (NamespaceService.isHeartbeatNamespace(topicName.getNamespaceObject()) || isSelf(topicName)) {
             return CompletableFuture.completedFuture(null);
         }
-        return sendTopicPolicyEvent(topicName, ActionType.DELETE, null);
+        return pulsarService.getNamespaceService().checkTopicExists(NamespaceEventsSystemTopicFactory
+                .getEventsTopicName(topicName.getNamespaceObject())).thenCompose(topicExistsInfo -> {
+            if (topicExistsInfo.isExists()) {
+                return CompletableFuture.completedFuture(null);
+            }
+            return sendTopicPolicyEvent(topicName, ActionType.DELETE, null);

Review Comment:
   Added brief code comment. And I found a bug `if (topicExistsInfo.isExists()) -> if (!topicExistsInfo.isExists())` ❤️ 



-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#discussion_r1793346866


##########
bin/ledgers.md:
##########


Review Comment:
   mistakenly added to PR?



-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2452557481

   @poorbarcode Do you have a chance to take a look at #23543 ? It's a deadlock in SystemTopicBasedTopicPoliciesService which happens also after #23522 has been merged. The deadlock might be completely unrelated to the recent changes, but since you know SystemTopicBasedTopicPoliciesService well, I'd appreciate it if you can take a look at the thread dump and the deadlocks.


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2441916384

   It appears that this PR failed the test in branch-3.3.
   
   https://github.com/heesung-sn/pulsar/actions/runs/11535492430/job/32111441443


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#discussion_r1793346866


##########
bin/ledgers.md:
##########


Review Comment:
   delete?



-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#discussion_r1793349811


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -133,7 +133,13 @@ public CompletableFuture<Void> deleteTopicPoliciesAsync(TopicName topicName) {
         if (NamespaceService.isHeartbeatNamespace(topicName.getNamespaceObject()) || isSelf(topicName)) {
             return CompletableFuture.completedFuture(null);
         }
-        return sendTopicPolicyEvent(topicName, ActionType.DELETE, null);
+        return pulsarService.getNamespaceService().checkTopicExists(NamespaceEventsSystemTopicFactory
+                .getEventsTopicName(topicName.getNamespaceObject())).thenCompose(topicExistsInfo -> {
+            if (topicExistsInfo.isExists()) {
+                return CompletableFuture.completedFuture(null);
+            }
+            return sendTopicPolicyEvent(topicName, ActionType.DELETE, null);

Review Comment:
   please add a brief code comment about the reason for this logic.



-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari closed pull request #23360: [fix] [broker] topics failed to delete after remove cluster from replicated clusters set and caused OOM
URL: https://github.com/apache/pulsar/pull/23360


-- 
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] topics failed to delete after remove cluster from replicated clusters set and caused OOM [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23360:
URL: https://github.com/apache/pulsar/pull/23360#issuecomment-2419355918

   I wonder if this is causing #23474 which currently blocks Pulsar CI. There's PR #23478 to disable OneWayReplicatorTestBase tests the problem is addressed. @poorbarcode do you have a chance to fix the problem?


-- 
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