hangc0276 commented on code in PR #3940:
URL: https://github.com/apache/bookkeeper/pull/3940#discussion_r1184476716
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -943,9 +943,6 @@ public Iterable<Long> getActiveLedgersInRange(long
firstLedgerId, long lastLedge
@Override
public void updateEntriesLocations(Iterable<EntryLocation> locations)
throws IOException {
- // Trigger a flush to have all the entries being compacted in the db
storage
- flush();
-
entryLocationIndex.updateLocations(locations);
Review Comment:
In fact, the data accumulated in `writeCacheBeingFlushed` is new data,
because we are compacting the old data.
Refer to:
> #### Behavior Change
> After we removed the `flush the current ledger storage write cache` step,
it brings one behavior change.
https://github.com/apache/bookkeeper/blob/c765aea600fde1a8ac7cb8a53a1157a372026894/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L947
> Let me take one case for example.
> Timeline: [t1, t2, t3]
t1: One entry (LedgerId = 1, EntryId = 1) with value "test-v1" written into
the bookie from one bookie client
t2: The entry (LedgerId = 1, EntryId = 1) with new value "test-new-v1"
written into the bookie from one bookie client
t3: Bookie triggers compaction to compact the entry written in t1, the entry
(LedgerId = 1, EntryId = 1) with value "test-v1" will be written into the
current entry log file, and update the entry's lookup index.
> ##### Before this change
> Due to the compactor flushing the current ledger storage write cache
before updating the entry's lookup index, the updated value of the entry
"test-new-v1" in `t2` will be flushed into storage and removed from the write
cache. If we get the entry (LedgerId = 1, EntryId = 1) after `t3`, we will get
the old value `test-v1`
> ##### After this change
> Due to this PR removed the compactor flushing the current ledger storage
write cache, it has two cases:
> - If `t3` happens when `t2`'s updated data is still located in the ledger
storage's write cache, the new data updated in `t2` will override the old data
written in t3. When we get the entry (LedgerId = 1, EntryId = 1), we will get
the new value `test-new-v1`
> - If `t3` happens when `t2`'s updated data has been flushed into the entry
log file and removed from the write cache, the old entry written in `t3` will
override the new entry written in `t2`. When we get the entry (LedgerId = 1,
EntryId=1), we will get the old value `test-v1`.
> IMO, we should always return the new value `test-new-v1`, not the old
value `test-v1`. If we need to make sure getting the entry always returns the
new value, we need more checks in writing the old value in the compaction stage.
> In Pulsar's general case, updating the entry's value won't happen.
--
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]
