reddycharan commented on a change in pull request #1281: Issue #570:
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r181626498
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
##########
@@ -788,89 +810,392 @@ 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;
+
+ /*
+ * flush both current and rotated logs.
+ */
+ void flush() throws IOException;
+
+ /*
+ * close current logs.
+ */
+ void close() throws IOException;
+
+ /*
+ * force close current logs.
+ */
+ void forceClose();
+
/*
- * 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.
*
- * 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.
+ */
+ void prepareSortedLedgerStorageCheckpoint(long numBytesFlushed) throws
IOException;
+
+ /*
+ * 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();
+
+ /*
+ * 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.
+ *
+ * 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 List<BufferedLogChannel> rotatedLogChannels;
+
+ EntryLogManagerBase() {
+ rotatedLogChannels = new
CopyOnWriteArrayList<BufferedLogChannel>();
Review comment:
regarding rotatedlogs i don't see the need of having separate implementation
for EntryLogManagerForSingleEntryLog and EntryLogManagerForEntryLogPerLedger
thats why I kept it in EntryLogManagerBase. I don't have strong opinion on
making this collection set or list, thats why I changed it back to List as we
discussed (to minimize the change in behavior).
But for the sake of correctness and simplicity it has to be concurrentlist.
for eg:
In this line 'logChannelsToFlush' is called in the log line, which would in
turn call toString method of List and toString method of List returns "the
string representation consists of a list of the collection's elements in the
order they are returned by its iterator". So while iterator is iterating if an
entrylog is added to rotatedLogChannels, LinkedList iterator will fail because
"if the list is structurally modified at any time after the iterator is
created, in any way except through the Iterator's own remove or add methods,
the iterator will throw a ConcurrentModificationException."
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java#L485
And because of CopyOnWriteArrayList, flushRotatedLogs method becomes fairly
simple and straightforward.
Also in our case "rotatedLogChannels" list will be modified only when
entrylog is rotated or when flushRotatedLogs is called, which is very less
frequent. So having CopyOnWriteArrayList isn't going to change the behavior in
any considerable way.
----------------------------------------------------------------
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
