You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "eolivelli (via GitHub)" <gi...@apache.org> on 2023/05/02 07:56:18 UTC
[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3940: Improve compaction performance
eolivelli commented on code in PR #3940:
URL: https://github.com/apache/bookkeeper/pull/3940#discussion_r1182194992
##########
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:
Maybe we should update the comments at here:
https://github.com/apache/bookkeeper/blob/f5455f01584b1b0a592f020eed49d3cb774da0a9/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L1040
##########
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:
For the standard compactor we are calling `EntryLogger.flush() `here before calling `updateEntriesLocations`
https://github.com/apache/bookkeeper/blob/405e72acf42bb1104296447ea8840d805094c787/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogCompactor.java#L120
My understanding is that with this change we are not flushing the index on RocksDB before actually updating the locations and we will wait for the next flush to happen.
In this [place](https://github.com/apache/bookkeeper/blob/ceba60565cf7cb438e9be4ab7416a2808b9168a1/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/EntryLocationIndex.java#L183) we flush the batch of writes related to the new entry locations passed to `updateEntriesLocations`
I wonder if we could risk to write these updates and then at the next flush overwrite the locations for the same entries with old data accumulated in `writeCacheBeingFlushed` https://github.com/apache/bookkeeper/blob/f5455f01584b1b0a592f020eed49d3cb774da0a9/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L817
--
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@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org