reddycharan commented on a change in pull request #1281: Issue #570:
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r180555218
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
##########
@@ -173,7 +175,7 @@ public void checkpoint(final Checkpoint checkpoint) throws
IOException {
// 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();
+ entryLogger.rollLogs();
Review comment:
@sijie i'm flexible here to make the change but
1) to begin with, from what i understand, this check (numBytesFlushed > 0)
and rollLogs call is not needed. Since entries which come before checkpoint are
already flushed from memtable (because of the way
sortedledgerstorage.checkpoint) is called.
2) im not super convinced with moving from rollLogs method in interface to
prepareCheckpoint, because this has meaning/use only for sortedledgerstorage
and for single entrylog manager and having such a generic method name
"prepareCheckpoint" seems to be a stretch.
3) in the method signature - "void prepareCheckpoint(Checkpoint checkpoint,
long numBytesFlushedBetweenCheckpoint)" 'checkpoint' is not needed and we can
remove it. parameter name 'numBytesFlushedBetweenCheckpoint' is not appropriate
since it is just bytes added between previous flush and this checkpoint.
----------------------------------------------------------------
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:
us...@infra.apache.org
With regards,
Apache Git Services
