BewareMyPower commented on PR #24346:
URL: https://github.com/apache/pulsar/pull/24346#issuecomment-3076800150

   I tried to understand the motivation by adding a test to 
`PendingReadsManagerTest`:
   
   ```java
           final var pendingReadsManager = new 
PendingReadsManager(rangeEntryCache);
           final var readFutures = new 
ArrayList<CapturingReadEntriesCallback>();
           final BiConsumer<Long, Long> readEntries = (firstEntry, lastEntry) 
-> {
               final var callback = new CapturingReadEntriesCallback();
               pendingReadsManager.readEntries(lh, firstEntry, lastEntry, 
false, callback, CTX);
               readFutures.add(callback);
           };
           final BiFunction<Long, Long, PreparedReadFromStorage> 
mockReadFromStorage = (firstEntry, lastEntry) ->
                   prepareReadFromStorage(lh, rangeEntryCache, firstEntry, 
lastEntry, false);
   
           final var read0 = mockReadFromStorage.apply(10L, 70L);
           readEntries.accept(10L, 70L);
           final var read1 = mockReadFromStorage.apply(80L, 100L);
           readEntries.accept(80L, 100L);
           final var read2 = mockReadFromStorage.apply(71L, 100L); // NOTE: 
it's not [71, 79]
           readEntries.accept(10L, 100L);
   
           read1.storageReadCompleted();
           read0.storageReadCompleted();
           read2.storageReadCompleted();
           FutureUtil.waitForAll(readFutures).get();
   ```
   
   See the `NOTE` comment, when I debugged the test, I found this description 
is wrong:
   
   > The third read will be splitted into [10,70], [71, 79], [80, 100].
   
   Instead, the 3rd read will just be split into:
   - A pending read on `[10, 70]`
   - A new read on `[71, 100]`
   
   The reason is for simplicity, only 1 `PendingRead` can be reused for a new 
`PendingReadsManager#readEntries` call.
   
   ```java
       private record FindPendingReadOutcome(PendingRead pendingRead, // NOTE: 
it's a single PendingRead
                                             PendingReadKey missingOnLeft, 
PendingReadKey missingOnRight) {
   ```
   
   Basically, the motivation is wrong, how can you say it's a clear 
argumentation? The root cause is that **there is no test or code example**, so 
the explanation might not match the actual behavior.


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to