BewareMyPower commented on code in PR #15663:
URL: https://github.com/apache/pulsar/pull/15663#discussion_r881366047


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2551,29 +2551,46 @@ public void readEntryFailed(ManagedLedgerException 
exception, Object ctx) {
                     }, null);
             return future;
         } else {
-            Long ledgerId = ((ManagedCursorContainer) 
ledger.getCursors()).getSlowestReaderPosition().getLedgerId();
+            PositionImpl slowestPosition = ((ManagedCursorContainer) 
ledger.getCursors()).getSlowestReaderPosition();
             try {
-                
org.apache.bookkeeper.mledger.proto.MLDataFormats.ManagedLedgerInfo.LedgerInfo
-                        ledgerInfo = ledger.getLedgerInfo(ledgerId).get();
-                if (ledgerInfo != null && ledgerInfo.hasTimestamp() && 
ledgerInfo.getTimestamp() > 0
-                        && ((ManagedLedgerImpl) ledger).getClock().millis() - 
ledgerInfo.getTimestamp()
-                        > backlogQuotaLimitInSecond * 1000) {
-                    if (log.isDebugEnabled()) {
-                        log.debug("Time based backlog quota exceeded, quota 
{}, age of ledger "
-                                        + "slowest cursor currently on {}", 
backlogQuotaLimitInSecond * 1000,
-                                ((ManagedLedgerImpl) 
ledger).getClock().millis() - ledgerInfo.getTimestamp());
-                    }
-                    return CompletableFuture.completedFuture(true);
-                } else {
-                    return CompletableFuture.completedFuture(false);
-                }
+                return 
slowestReaderTimeBasedBacklogQuotaCheck(slowestPosition);
             } catch (Exception e) {
                 log.error("[{}][{}] Error reading entry for precise time based 
backlog check", topicName, e);
                 return CompletableFuture.completedFuture(false);
             }
         }
     }
 
+    private CompletableFuture<Boolean> 
slowestReaderTimeBasedBacklogQuotaCheck(PositionImpl slowestPosition)
+            throws ExecutionException, InterruptedException {
+        int backlogQuotaLimitInSecond = 
getBacklogQuota(BacklogQuotaType.message_age).getLimitTime();
+        Long ledgerId = slowestPosition.getLedgerId();
+        if (((ManagedLedgerImpl) 
ledger).getLedgersInfo().lastKey().equals(ledgerId)) {
+            return CompletableFuture.completedFuture(false);
+        }
+        int result;
+        
org.apache.bookkeeper.mledger.proto.MLDataFormats.ManagedLedgerInfo.LedgerInfo
+                ledgerInfo = ledger.getLedgerInfo(ledgerId).get();
+        if (ledgerInfo != null && ledgerInfo.hasTimestamp() && 
ledgerInfo.getTimestamp() > 0
+                && ((ManagedLedgerImpl) ledger).getClock().millis() - 
ledgerInfo.getTimestamp()
+                > backlogQuotaLimitInSecond * 1000 && (result = 
slowestPosition.compareTo(
+                new PositionImpl(ledgerInfo.getLedgerId(), 
ledgerInfo.getEntries() - 1))) <= 0) {
+            if (result < 0) {

Review Comment:
   Yes. I also found it. So I think it's controversial here and marked it as 
resolved.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2551,29 +2551,46 @@ public void readEntryFailed(ManagedLedgerException 
exception, Object ctx) {
                     }, null);
             return future;
         } else {
-            Long ledgerId = ((ManagedCursorContainer) 
ledger.getCursors()).getSlowestReaderPosition().getLedgerId();
+            PositionImpl slowestPosition = ((ManagedCursorContainer) 
ledger.getCursors()).getSlowestReaderPosition();
             try {
-                
org.apache.bookkeeper.mledger.proto.MLDataFormats.ManagedLedgerInfo.LedgerInfo
-                        ledgerInfo = ledger.getLedgerInfo(ledgerId).get();
-                if (ledgerInfo != null && ledgerInfo.hasTimestamp() && 
ledgerInfo.getTimestamp() > 0
-                        && ((ManagedLedgerImpl) ledger).getClock().millis() - 
ledgerInfo.getTimestamp()
-                        > backlogQuotaLimitInSecond * 1000) {
-                    if (log.isDebugEnabled()) {
-                        log.debug("Time based backlog quota exceeded, quota 
{}, age of ledger "
-                                        + "slowest cursor currently on {}", 
backlogQuotaLimitInSecond * 1000,
-                                ((ManagedLedgerImpl) 
ledger).getClock().millis() - ledgerInfo.getTimestamp());
-                    }
-                    return CompletableFuture.completedFuture(true);
-                } else {
-                    return CompletableFuture.completedFuture(false);
-                }
+                return 
slowestReaderTimeBasedBacklogQuotaCheck(slowestPosition);
             } catch (Exception e) {
                 log.error("[{}][{}] Error reading entry for precise time based 
backlog check", topicName, e);
                 return CompletableFuture.completedFuture(false);
             }
         }
     }
 
+    private CompletableFuture<Boolean> 
slowestReaderTimeBasedBacklogQuotaCheck(PositionImpl slowestPosition)
+            throws ExecutionException, InterruptedException {
+        int backlogQuotaLimitInSecond = 
getBacklogQuota(BacklogQuotaType.message_age).getLimitTime();
+        Long ledgerId = slowestPosition.getLedgerId();
+        if (((ManagedLedgerImpl) 
ledger).getLedgersInfo().lastKey().equals(ledgerId)) {
+            return CompletableFuture.completedFuture(false);
+        }
+        int result;
+        
org.apache.bookkeeper.mledger.proto.MLDataFormats.ManagedLedgerInfo.LedgerInfo
+                ledgerInfo = ledger.getLedgerInfo(ledgerId).get();
+        if (ledgerInfo != null && ledgerInfo.hasTimestamp() && 
ledgerInfo.getTimestamp() > 0
+                && ((ManagedLedgerImpl) ledger).getClock().millis() - 
ledgerInfo.getTimestamp()
+                > backlogQuotaLimitInSecond * 1000 && (result = 
slowestPosition.compareTo(
+                new PositionImpl(ledgerInfo.getLedgerId(), 
ledgerInfo.getEntries() - 1))) <= 0) {
+            if (result < 0) {

Review Comment:
   Yes. I also realized it. So I think it's controversial here and marked it as 
resolved.



-- 
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-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to