sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r180514001
########## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java ########## @@ -788,89 +852,340 @@ private long readLastLogId(File f) { } } - /** - * Flushes all rotated log channels. After log channels are flushed, - * move leastUnflushedLogId ptr to current logId. - */ - void checkpoint() throws IOException { - flushRotatedLogs(); + interface EntryLogManager { + + /* + * add entry to the corresponding entrylog and return the position of + * the entry in the entrylog + */ + long addEntry(Long ledger, ByteBuf entry, boolean rollLog) throws IOException; + + /* + * gets the active logChannel with the given entryLogId. null if it is + * not existing. + */ + BufferedLogChannel getCurrentLogIfPresent(long entryLogId); + + /* + * Returns eligible writable ledger dir for the creation next entrylog + */ + File getDirForNextEntryLog(List<File> writableLedgerDirs); + + /* + * Do the operations required for checkpoint. + */ + void checkpoint() throws IOException; + + /* + * roll entryLogs. + */ + void rollLogs() throws IOException; + + /* + * flush rotated logs. + */ + void flushRotatedLogs() throws IOException; + + /* + * flush current logs. + */ + void flushCurrentLogs() throws IOException; + + /* + * close current logs. + */ + void closeCurrentLogs() throws IOException; + + /* + * force close current logs. + */ + void forceCloseCurrentLogs(); + + /* + * this method should be called before doing entrymemtable flush, it + * would save the state of the entrylogger before entrymemtable flush + * and commitEntryMemTableFlush would take appropriate action after + * entrymemtable flush. + */ + void prepareEntryMemTableFlush(); + /* - * In the case of entryLogPerLedgerEnabled we need to flush both - * rotatedlogs and currentlogs. This is needed because syncThread - * periodically does checkpoint and at this time all the logs should - * be flushed. + * this method should be called after doing entrymemtable flush,it would + * take appropriate action after entrymemtable flush depending on the + * current state of the entrylogger and the state of the entrylogger + * during prepareEntryMemTableFlush. * - * TODO: When EntryLogManager is introduced in the subsequent sub-tasks of - * this Issue, I will move this logic to individual implamentations of - * EntryLogManager and it would be free of this booalen flag based logic. + * It is assumed that there would be corresponding + * prepareEntryMemTableFlush for every commitEntryMemTableFlush and both + * would be called from the same thread. * + * returns boolean value indicating whether EntryMemTable should do checkpoint + * after this commit method. */ - if (entryLogPerLedgerEnabled) { - flushCurrentLog(); - } + boolean commitEntryMemTableFlush() throws IOException; } - void flushRotatedLogs() throws IOException { - List<BufferedLogChannel> channels = null; - long flushedLogId = INVALID_LID; - synchronized (this) { - channels = logChannelsToFlush; - logChannelsToFlush = null; + abstract class EntryLogManagerBase implements EntryLogManager { + final Set<BufferedLogChannel> rotatedLogChannels; Review comment: I am not sure we need to put this rotatedLogChannels in this base class. for two reasons, 1) there is really no commonality between single-log manager and per-ledger-log manager around checkpoint/rotation. I don't think it is necessarily to put them in the single model. 2) I would prefer using an ordered structure (before this change, it is a `list`), because entry log file rotation in single-log manager is in order and checkpoint logic is in order. changing single-log manager to use `Set` could expose unknown potential bugs, which is a risky change to me. ---------------------------------------------------------------- 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