[GitHub] [pulsar] sijie commented on a change in pull request #5809: [Issue:5669] Fix the ledgerID not found cause NPE

[GitBox <gi...@apache.org>]

sijie commented on a change in pull request #5809: [Issue:5669] Fix the ledgerID not found cause NPE
URL: https://github.com/apache/pulsar/pull/5809#discussion_r355149324
 
 

 ##########
 File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
 ##########
 @@ -1790,8 +1790,12 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
         }
     }
 
-    PositionImpl startReadOperationOnLedger(PositionImpl position) {
-        long ledgerId = ledgers.ceilingKey(position.getLedgerId());
+    PositionImpl startReadOperationOnLedger(PositionImpl position, ReadEntriesCallback callback) {
+        Long ledgerId = ledgers.ceilingKey(position.getLedgerId());
+        if (null == ledgerId) {
+            callback.readEntriesFailed(new ManagedLedgerNotFoundException("read ledger failed, the ledgerID is null"), null);
 
 Review comment:
   1) we should not use ManagedLedgerNotFoundExcception. ManagedLedgerNotFoundExcception is used when a managed ledger is not found, not a ledger in the managed ledger is not found
   
   2) the error message should be improved. `The ceilingKey(K key) method is used to return the least key greater than or equal to the given key, or null if there is no such key.` that means in this managed ledger there is no ledger greater than or equal to `position.getLedgerId()`.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services