Thank you for the comments and reviews Oystein.
>1. AsynchronousLogShipper#shipALogChunk: If resending of
> failedChunk fails, the current code will set failedChunk to null.
> Is that intentional?
I guess you are referring to failedChunk being assigned mesg. When
failedChunk transmission fails then mesg would also be null.
It was my mistake and was not intentional.
I have overcome this by doing
failedChunk = (mesg==null) ? failedChunk : mesg;
mesg is null would imply that a transmission of the failedChunk was being
attempted and failedChunk would not be lost now.
>2. MasterController#setupConnection: Why are IOEXcecptions not
> reported? Maybe the IOException could contain useful information
> to figure out a network problem.
I initially thought of reporting IOExceptions here, but IOExceptions
cause setupConnection to return false and a next reconnect is attempted
in half a second. Reporting this failure in log would cause a
IOException to be printed in the log every half a second. Would'nt this
amount to flooding the log? All these IOEXceptions being registered would
contain the same information stating a socket connection exception. So I was
afraid that it would contain the same information being repeated too many times.
> a) Do you depend on the thread to be interrupted before you give up
> attempting to start replication? (Same issue with
Yes, the attempt to connect to the slave will persist until it is interrupted.
One thing I can do is to ensure that the attempts are stopped when the user calls
stopreplication. I have attempted to link stopMaster() with this. I did this because
upon retrospection it did not seen right to me that a network connection is attempted
while the user has called stopreplication.
> b) InterruptedExceptions are normally trapped since they occur when
> some code intentionally interrupts the thread.
I was throwing the interrupted exception obtained from Thread.currentThread().sleep(500);.
I have changed it to simply log the exception and throw it no more.
> a) The setting of the stop state is not synchronized with the log
> shipper thread. Hence, you have no guarantee for when the log
> shipper thread will see that the stop field has changed. If you
> make AsynchronousLogShipper#shipALogChunk volatile, it will be
> guaranteed that updates to the field are visible to other
> threads. (An alternative is to make stopLogShipment
> synchronized, but that is probably more expensive).
> b) Maybe you should interrupt the log shipper thread to make sure
> it stops immediately?
I have interrupted the log shipper thread and consequently removed the stopShipping
check from the run() method. I instead have introduced a while(true) loop. I have however
retained the stopShipping check inorder to ensure that a forceFlush call after stopreplication
does not succeed.
>5. Do you plan to internationalize the derby.log messages in a later
Yes, I was hoping that this could be done in a follow up patch if this would be OK
>6. I suggest you make a method that log the error message, print the
> stack and stop the master, so you do not have to repeat that many
Introduced printStackAndStopMaster that does the sequence. I thought I could
move the printErrorStack code also in but doing that seemed to clutter a three
step process being modularized. So decided to let it remain this way.
>7. There is an unused import of SanityManager in MasterController.java
I have removed this unused import.
>8. MasterController#startMaster(handleExceptions: Thread.sleep is
> static, so there is no need to call currentThread().
changed it to Thread.sleep in handleExceptions also.