You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "horizonzy (via GitHub)" <gi...@apache.org> on 2023/08/17 07:10:26 UTC

[GitHub] [pulsar] horizonzy opened a new pull request, #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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

   ### Motivation
   When the user config `-Dbookkeeper.metadata.client.drivers=org.apache.pulsar.metadata.bookkeeper.PulsarMetadataClientDriver`, the AutoRecovery will use PulsarLedgerUnderreplicationManager.
   
   
   https://github.com/apache/bookkeeper/blob/f30ff4f2ad4778f1f73b29872e2a95adb22ca116/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L396-L397.
   
   In Auditor start, it will register a UnderReplicatedLedgersChangedCb to PulsarLedgerUnderreplicationManager,
   PulsarLedgerUnderreplicationManager will register a watcher to watch the zk event. 
   
   When the event path matches `underreplication`, callback UnderReplicatedLedgersChangedCb, the callback executor is metadata-store executor.
   https://github.com/apache/pulsar/blob/0cb1c780412ca3cc1d421fe6ebb5de95cf7392fa/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java#L307-L323
   
   
   In UnderReplicatedLedgersChangedCb, it will get all the `underreplication` ledgers from the metadata store, and this is a sync operation. 
   
   https://github.com/apache/bookkeeper/blob/f30ff4f2ad4778f1f73b29872e2a95adb22ca116/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L561-L569
   
   
   https://github.com/apache/pulsar/blob/0cb1c780412ca3cc1d421fe6ebb5de95cf7392fa/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java#L427-L479
   
   line_448, use future.get(), it's sync.
   
   There is the stack file:
   [jastack.txt](https://github.com/apache/pulsar/files/12366413/jastack.txt)
   
   
   ### 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 -->
   - [ ] `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] Technoboy- closed pull request #21010: [fix][auto-recovery] Fix deadlock in AutoRecovery.

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #21010: [fix][auto-recovery] Fix deadlock in AutoRecovery.
URL: https://github.com/apache/pulsar/pull/21010


-- 
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 #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   Can we make the metrics update task runs in another thread?



-- 
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] hangc0276 commented on a diff in pull request #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   The interface is defined in BookKeeper and we will deprecate it in next BookKeeper major version.



-- 
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 #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   Thanks explain this to me.



-- 
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] Technoboy- merged pull request #21010: [fix][meta] Fix deadlock in AutoRecovery.

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


-- 
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] horizonzy commented on a diff in pull request #21010: [fix][meta] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)
             throws ReplicationException.UnavailableException {
-        log.debug("notifyUnderReplicationLedgerChanged()");
-        store.registerListener(e -> {
-            if (e.getType() == NotificationType.Deleted && ID_EXTRACTION_PATTERN.matcher(e.getPath()).find()) {
-                cb.operationComplete(0, null);
-            }
-        });
+        //The store listener callback executor is metadata-store executor,
+        //in cb.operationComplete(0, null), it will get all underreplication ledgers from metadata-store, it's sync
+        //operation. So it's a deadlock.
+//        store.registerListener(e -> {
+//            if (e.getType() == NotificationType.Deleted && ID_EXTRACTION_PATTERN.matcher(e.getPath()).find()) {
+//                cb.operationComplete(0, null);
+//            }
+//        });

Review Comment:
   I want the user to know there may be a deadlock when they see this code.  I agree with you that we should left the comments for user to let them know that PulsarLedgerUnderreplicationManager#notifyUnderReplicationLedgerChanged will be removed at next bk major release. But it's already merged, so if user confused, they could see this pr link to get the 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

[GitHub] [pulsar] hangc0276 commented on a diff in pull request #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   The interface is defined in BookKeeper and we will deprecated in BookKeeper next major version.



-- 
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 #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   Make sense. I think sending an email to notify others why we did this change is better.



-- 
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 #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   Do you think we should consider deprecating this 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] horizonzy commented on a diff in pull request #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   I draft a discussion in bk dev. https://lists.apache.org/thread/1xl3hr2cpyd5xh9kozbx5xlfsjsg3f4h



-- 
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] codecov-commenter commented on pull request #21010: [fix][auto-recovery] Fix deadlock in AutoRecovery.

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

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21010?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#21010](https://app.codecov.io/gh/apache/pulsar/pull/21010?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9c140cc) into [master](https://app.codecov.io/gh/apache/pulsar/commit/0cb1c780412ca3cc1d421fe6ebb5de95cf7392fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0cb1c78) will **increase** coverage by `0.33%`.
   > Report is 6 commits behind head on master.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/21010/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/21010?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #21010      +/-   ##
   ============================================
   + Coverage     33.56%   33.89%   +0.33%     
   - Complexity    12198    12408     +210     
   ============================================
     Files          1621     1621              
     Lines        126970   128058    +1088     
     Branches      13857    14164     +307     
   ============================================
   + Hits          42618    43407     +789     
   - Misses        78748    78960     +212     
   - Partials       5604     5691      +87     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21010/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/21010/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.15% <ø> (-0.09%)` | :arrow_down: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/21010/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `∅ <ø> (?)` | |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21010/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.51% <ø> (+0.41%)` | :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 Changed](https://app.codecov.io/gh/apache/pulsar/pull/21010?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...ookkeeper/PulsarLedgerUnderreplicationManager.java](https://app.codecov.io/gh/apache/pulsar/pull/21010?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLW1ldGFkYXRhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvbWV0YWRhdGEvYm9va2tlZXBlci9QdWxzYXJMZWRnZXJVbmRlcnJlcGxpY2F0aW9uTWFuYWdlci5qYXZh) | `0.00% <ø> (ø)` | |
   
   ... and [49 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21010/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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] codelipenghui commented on a diff in pull request #21010: [fix][meta] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)
             throws ReplicationException.UnavailableException {
-        log.debug("notifyUnderReplicationLedgerChanged()");
-        store.registerListener(e -> {
-            if (e.getType() == NotificationType.Deleted && ID_EXTRACTION_PATTERN.matcher(e.getPath()).find()) {
-                cb.operationComplete(0, null);
-            }
-        });
+        //The store listener callback executor is metadata-store executor,
+        //in cb.operationComplete(0, null), it will get all underreplication ledgers from metadata-store, it's sync
+        //operation. So it's a deadlock.
+//        store.registerListener(e -> {
+//            if (e.getType() == NotificationType.Deleted && ID_EXTRACTION_PATTERN.matcher(e.getPath()).find()) {
+//                cb.operationComplete(0, null);
+//            }
+//        });

Review Comment:
   Why not remove these lines and add an explanation of why we should keep the method empty.



-- 
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] horizonzy commented on a diff in pull request #21010: [fix] [auto-recovery] Fix deadlock in AutoRecovery.

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


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerUnderreplicationManager.java:
##########
@@ -978,11 +978,13 @@ public long getReplicasCheckCTime() throws ReplicationException.UnavailableExcep
     @Override
     public void notifyUnderReplicationLedgerChanged(BookkeeperInternalCallbacks.GenericCallback<Void> cb)

Review Comment:
   It's unnecessary, the callback is heavy for the zookeeper, and we plan to remove UnderReplicatedLedgersChangedCb.



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