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_r153363806
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
##########
@@ -465,6 +437,9 @@ public void onRotateEntryLog() {
// TODO: we could consider remove checkpointSource and
checkpointSouce#newCheckpoint
// later if we provide kind of LSN (Log/Journal Squeuence Number)
// mechanism when adding entry. {@link
https://github.com/apache/bookkeeper/issues/279}
- checkpointHolder.setNextCheckpoint(checkpointSource.newCheckpoint());
+ if (null != checkpointer) {
Review comment:
it is very confusing that you added 'checkpointer' variable both in
InterleavedLedgerStorage and its subclass - SortedLedgerStorage. It is easy to
get lost or misunderstand.
2.a) I'm not convinced if it is right approach to have different behavior
depending on the type of LedgerStorage. In the case of
InterleavedLedgerStorage, in the event of 'onRotateEntryLog' you trigger
checkpoint but in the case of SortedLedgerStorage it is noop (you swallow it).
2.b) If you dont agree with what I mentioned in 2.a), then I would like to
ask is it necessary to have EntryLogListener.onRotateEntryLog? cannt we have
something similar to CacheCallback.onSizeLimitReached in
InterleavedLedgerStorage also and get rid of EntryLogListener?
----------------------------------------------------------------
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
