[
https://issues.apache.org/jira/browse/BOOKKEEPER-610?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13655612#comment-13655612
]
Rakesh R commented on BOOKKEEPER-610:
-------------------------------------
Thanks [[email protected]] for the patch and nice improvement work.
Please see the following comments.
- Please correct the 'getLogger(BookieJournalTest.class)'
{code}
+public class TestSyncThread {
+ static Logger LOG = LoggerFactory.getLogger(BookieJournalTest.class);
{code}
- Please add timeout to the below testcases.
{code}
+ @Test
+ public void testSyncThreadDisksFull() throws Exception {
+ @Test
+ public void testSyncThreadShutdownOnError() throws Exception {
{code}
- Pass exception object to the logger, would be useful when debugging.
{code}
LOG.error("No writeable ledger directories", e);
{code}
- Please remove unused variable 'flushFailed'
{code}
public void checkpoint(Checkpoint checkpoint) {
boolean flushFailed = false;
{code}
- Just a suggestion :
flush() is getting called on shutdown(). I could see zk is already closed
before this call and here if enters to 'dirsListener.allDisksFull();'
will always throws unnecessary zk exceptions. IMO, flush() doesn't required to
handle any exceptions, just throw back to the callee. Whats do you say?
{code}
private void flush() {
try {
.....
} catch (NoWritableLedgerDirException e) {
LOG.error("No writeable ledger directories");
dirsListener.allDisksFull();
} catch (IOException e) {
LOG.error("Exception flushing ledgers", e);
}
}
{code}
-Rakesh
> Make SyncThread use an executor
> -------------------------------
>
> Key: BOOKKEEPER-610
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-610
> Project: Bookkeeper
> Issue Type: Bug
> Reporter: Ivan Kelly
> Assignee: Ivan Kelly
> Fix For: 4.3.0
>
> Attachments: 0001-BOOKKEEPER-610-Make-SyncThread-use-an-executor.patch
>
>
> Currently we have a bunch of boolean variables to control the lifecycle of
> the SyncThread. We're effectively replicating what an Executor does, so we
> should just use an executor.
--
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
