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/10/05 19:58:36 UTC

[GitHub] [pulsar] 315157973 opened a new pull request, #17911: Fix mutex never releasing when trimming

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

   
   ### Motivation
   During the Ledger trimming process, if the number of BK nodes is insufficient or the BK cluster is temporarily unavailable, the new Ledger will fail to be created.
   At this point, the TreeMap will return null, and `firstNonDeletedLedger` is the primitive type, so an NPE will be thrown, and the mutex will never be released
   
   ![image](https://user-images.githubusercontent.com/9758905/193498620-111a3a11-583a-49b7-bf51-382a986de2bf.png)
   
   
   ### Modifications
   Throw and catch the exception
   
   ### Verifying this change
   
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc-required` 
   (Your PR needs to update docs and you will update later)
   
   - [x] `doc-not-needed` 
   bug fix
   
   - [ ] `doc` 
   (Your PR contains doc changes)
   
   - [ ] `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] codelipenghui merged pull request #17911: [fix][broker]Fix mutex never released when trimming

Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17911:
URL: https://github.com/apache/pulsar/pull/17911


-- 
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] 315157973 commented on a diff in pull request #17911: [fix][broker]Fix mutex never released when trimming

Posted by GitBox <gi...@apache.org>.
315157973 commented on code in PR #17911:
URL: https://github.com/apache/pulsar/pull/17911#discussion_r990644224


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2652,15 +2659,19 @@ public void operationFailed(MetaStoreException e) {
      * This is to make sure that the `consumedEntries` counter is correctly updated with the number of skipped
      * entries and the stats are reported correctly.
      */
-    private void advanceCursorsIfNecessary(List<LedgerInfo> ledgersToDelete) {
+    @VisibleForTesting
+    void advanceCursorsIfNecessary(List<LedgerInfo> ledgersToDelete) throws ManagedLedgerNotFoundException {
         if (ledgersToDelete.isEmpty()) {
             return;
         }
 
         // need to move mark delete for non-durable cursors to the first ledger NOT marked for deletion
         // calling getNumberOfEntries latter for a ledger that is already deleted will be problematic and return
         // incorrect results
-        long firstNonDeletedLedger = ledgers.higherKey(ledgersToDelete.get(ledgersToDelete.size() - 1).getLedgerId());
+        Long firstNonDeletedLedger = ledgers.higherKey(ledgersToDelete.get(ledgersToDelete.size() - 1).getLedgerId());
+        if (firstNonDeletedLedger == null) {
+            throw new ManagedLedgerNotFoundException("First non deleted Ledger is not found");

Review Comment:
   Done
   



-- 
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] 315157973 closed pull request #17911: [fix][broker]Fix mutex never released when trimming

Posted by GitBox <gi...@apache.org>.
315157973 closed pull request #17911: [fix][broker]Fix mutex never released when trimming
URL: https://github.com/apache/pulsar/pull/17911


-- 
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 #17911: [fix][broker]Fix mutex never released when trimming

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17911:
URL: https://github.com/apache/pulsar/pull/17911#discussion_r990577133


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2652,15 +2659,19 @@ public void operationFailed(MetaStoreException e) {
      * This is to make sure that the `consumedEntries` counter is correctly updated with the number of skipped
      * entries and the stats are reported correctly.
      */
-    private void advanceCursorsIfNecessary(List<LedgerInfo> ledgersToDelete) {
+    @VisibleForTesting
+    void advanceCursorsIfNecessary(List<LedgerInfo> ledgersToDelete) throws ManagedLedgerNotFoundException {
         if (ledgersToDelete.isEmpty()) {
             return;
         }
 
         // need to move mark delete for non-durable cursors to the first ledger NOT marked for deletion
         // calling getNumberOfEntries latter for a ledger that is already deleted will be problematic and return
         // incorrect results
-        long firstNonDeletedLedger = ledgers.higherKey(ledgersToDelete.get(ledgersToDelete.size() - 1).getLedgerId());
+        Long firstNonDeletedLedger = ledgers.higherKey(ledgersToDelete.get(ledgersToDelete.size() - 1).getLedgerId());
+        if (firstNonDeletedLedger == null) {
+            throw new ManagedLedgerNotFoundException("First non deleted Ledger is not found");

Review Comment:
   A little confused for `ManagedLedgerNotFoundException`
   I think `LedgerNotExistException` is better for this case and easier to understand.
   



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