reddycharan commented on a change in pull request #1281: Issue #570:
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176652302
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
##########
@@ -167,14 +167,7 @@ public ByteBuf getEntry(long ledgerId, long entryId)
throws IOException {
@Override
public void checkpoint(final Checkpoint checkpoint) throws IOException {
- long numBytesFlushed = memTable.flush(this, checkpoint);
- if (numBytesFlushed > 0) {
- // if bytes are added between previous flush and this checkpoint,
- // it means bytes might live at current active entry log, we need
- // roll current entry log and then issue checkpoint to underlying
- // interleaved ledger storage.
- entryLogger.rollLog();
- }
+ memTable.flush(this, checkpoint);
Review comment:
First of all I’m not sure, why this is added with this commit
https://github.com/apache/bookkeeper/commit/81cbba3cf620f2a6df48c0da6ee1ac019de24fbc.
SortedLedgerStorage.checkpoint should be called from onSizeLimitReached’s
scheduler. In onSizeLimitReached’s scheduler, memtable is flushed completely
and if entrylog has reached its capacity then it will roll log. So here by the
time startCheckpoint for the checkpoint cp is called all the entries must be
flushed from memtable to entry log and the entry log must be rolled. So in this
checkpoint method, the entries which come before checkpoint cp are already
added to entry log and rolled over. So I don’t see the need of this extra
rollLog call.
Mainly I removed rollLog method, instead I introduced
rollLogsIfEntryLogLimitReached, which is not appropriate here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
