ivankelly commented on a change in pull request #1236: Issue #570: make changes
to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r173429077
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
##########
@@ -331,9 +333,16 @@ private void flushOrCheckpoint(boolean isCheckpointFlush)
}
try {
- // if it is just a checkpoint flush, we just flush rotated entry
log files
- // in entry logger.
- if (isCheckpointFlush) {
+ /*
+ * if it is just a checkpoint flush and if entryLogPerLedger is not
+ * enabled, then we just flush rotated entry log files in entry
+ * logger.
+ *
+ * In the case of entryLogPerLedgerEnabled we need to flush both
+ * rotatedlogs and currentlogs. Hence we call entryLogger.flush in
+ * the case of entrylogperledgerenabled.
+ */
+ if (isCheckpointFlush && !entryLogPerLedgerEnabled) {
Review comment:
So effectively, entryLogPerLedgerEnabled disables checkpoint() on
EntryLogger. Don't add another boolean flag in this method. There's already 2
in play, and this method needs to be refactored badly.
Instead, handle it in the constructor.
```
if (conf.isEntryLogPerLedgerEnabled()) {
entryLogger = new EntryLogger(conf, ledgerDirsManager, this) {
@Override
void checkpoint() throws IOException {
// Add comment explaining why you always need to flush for per
ledger
flush();
}
};
} else {
entryLogger = new EntryLogger(conf, ledgerDirsManager, this);
}
```
Eventually when the whole feature in, this can be refactored into it's own
entrylogger.
----------------------------------------------------------------
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
