[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`

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

Technoboy- commented on code in PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#discussion_r887441337


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2227,15 +2227,9 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
         }
     }
 
-    PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
+    PositionImpl startReadOperationOnLedger(PositionImpl position) {
         Long ledgerId = ledgers.ceilingKey(position.getLedgerId());
-        if (null == ledgerId) {
-            opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("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"), null);
-        }
-
-        if (ledgerId != position.getLedgerId()) {
+        if (ledgerId != null && ledgerId != position.getLedgerId()) {

Review Comment:
   The case where the ledgerId is null is really weird. We reconfirmed the logic. Although there is no problem in returning the position, do we need to add a log here? 
   @codelipenghui 



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