reddycharan commented on a change in pull request #677: Issue 659: Fix
Checkpoint logic in SortedLedgerStorage
URL: https://github.com/apache/bookkeeper/pull/677#discussion_r153383988
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
##########
@@ -146,14 +165,9 @@ public ByteBuf getEntry(long ledgerId, long entryId)
throws IOException {
}
@Override
- public Checkpoint checkpoint(final Checkpoint checkpoint) throws
IOException {
- Checkpoint lastCheckpoint = checkpointHolder.getLastCheckpoint();
- // if checkpoint is less than last checkpoint, we don't need to do
checkpoint again.
- if (lastCheckpoint.compareTo(checkpoint) > 0) {
- return lastCheckpoint;
- }
+ public void checkpoint(final Checkpoint checkpoint) throws IOException {
Review comment:
hmm..you still didn't explain me why we need to call
memtable.flush(this,checkpoint) here. I've couple of issues with this
1) SortLS.onSizeLimitReached -> SyncThread.checkpoint -> SortLS.checkpoint
is the only call hierarchy in production code for this method. In
SLS.onSizeLimitReached "memTable.flush(SortedLedgerStorage.this);" is already
called, so what is the point in calling it again?
2) Ok, lets assume there is relevant data in EntryMemTable for this
checkpoint, which is added after the last memtable.flush call (line 205)
(actually that's not the case), now if we call "memTable.flush(this,
checkpoint);" here, it would flush the data from memtable to current active
log, but the "super.checkpoint(checkpoint);" will flush only rotatedlogs but
not active current log, which defies your assumption/purpose
In summary, there is no point in calling "memTable.flush(this, checkpoint);"
here and there is no point in having 'checkpoint' argument to the checkpoint
method in LedgerStorage interface itself.
----------------------------------------------------------------
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
