[
https://issues.apache.org/jira/browse/BOOKKEEPER-564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13615435#comment-13615435
]
Ivan Kelly commented on BOOKKEEPER-564:
---------------------------------------
The patch seems to mix a couple of changes together, and doesn't actually make
it much easier to address BOOKKEEPER-572's problem. However, the general idea
of the changes is quite good. I have the following comments
- SyncThread javadoc is out of date now.
- SyncThread#startCheckpoint calls offsetCheckPoint directly. offsetCheckPoint
only called by startCheckpoint. offsetCheckPoint should be removed and the code
put directly in #startCheckpoint
- the names requestCheckpoint and startCheckpoint are too synonymous.
requestCheckpoint suggests that the checkpoint should begin at that point in
time. It would be better to rename requestCheckpoint to newCheckpoint.
- rename CheckpointProgress to Checkpointer
- in InterleavedLedgerStorage, assert checkPointer != null in the constructor.
It's not nice to be passing around nulls. Create a Null implementation of
checkpointer if you want not to use a full one in tests.
- InterleavedLedgerStorage#processEntry() is never used. Remove from this patch.
- flushOptional(boolean, boolean) is ugly. It's impossible to tell how
flushOptional(false, true) is different to flushOptional(true,false) by just
looking at it. For this reason, boolean parameters should be used very
sparingly. The force parameter is used incorrectly. In
LedgerCacheImpl#flushLedger(boolean), the parameter is used to specify whether
all indices should be flushed or only one. In fact, this is a perfect example
of why boolean parameters suck. I would suggest breaking flushOptional into two
methods and call them directly from #checkpoint() and #flush().
- In fact, I think you should get rid of separate #flush() and #checkpoint()
completely, as #flush only seems to be used when shutting down. So you can do
whatever flush is doing in the #shutdown() method.
- Why make entrylogger preallocation optional?
- #checkpoint.checkpointComplete(running); so we only cleanup old journals on
shutdown? what if we never shutdown? I think this job may be better served by a
background thread or something in the journal, and never exposed to the upper
level.
> Better checkpoint mechanism
> ---------------------------
>
> Key: BOOKKEEPER-564
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-564
> Project: Bookkeeper
> Issue Type: Improvement
> Components: bookkeeper-server
> Reporter: Sijie Guo
> Assignee: Sijie Guo
> Fix For: 4.3.0
>
> Attachments: BOOKKEEPER-564.patch
>
>
> Currently, SyncThread made a checkpoint too frequently, which affects
> performance. data is writing to entry logger file might be blocked by syncing
> same entry logger file, which affect bookie to achieve higher throughput. We
> could schedule checkpoint only when rotating an entry log file. so new
> incoming entries would be written to newer entry log file and old entry log
> file could be synced.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
