sijie commented on a change in pull request #1281: Issue #570: Introducing
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r177532672
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
##########
@@ -209,16 +211,39 @@ public void onSizeLimitReached(final Checkpoint cp)
throws IOException {
public void run() {
try {
LOG.info("Started flushing mem table.");
- long logIdBeforeFlush = entryLogger.getCurrentLogId();
+ long prevAllocLogIdBeforeFlush =
entryLogger.getPreviousAllocatedEntryLogId();
Review comment:
This change is not correct. Because it should be "current" log id, not
"previous allocated entry log id". That says the behavior is different between
preallocation enabled and disabled.
That leads me to think that current entry log manager interface is not
abstracted with right methods. because it abstracts a lot of methods around the
underlying files, which underlying files are kind of tight with corresponding
checkpointing and flushing logic. two different entry log manager can't really
fit in this interface or checkpointing logic as well.
So as what I said in my previous comment, we need to think about what is the
right methods to put in EntryLogManager.
> I would suggest the EntryLogManager should hide the details on how to
create log, allocate log,
> flush/checkpoint logs. the details such as preallocationLogId,
unflushedLogId and how to lock them should be > hidden to the implementation,
rather than defining a too finer interface 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
