[
https://issues.apache.org/jira/browse/DERBY-3064?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12531499
]
Øystein Grøvlen commented on DERBY-3064:
----------------------------------------
Thanks for addressing my comments. I have a few follw-up issues and
some issues that I did not think of the previous time:
9. I am not sure it is right to put the LogShipper in iapi. As far
as I can tell the MasterFactory will be the mediator for the rest
of the system with respect to replication. Hence, I do not think
it is necessary for LogShipper to be visible outside the master
package.
10. Along the same lines, maybe the AsynchronousLogShipper could
relate directly to the MasterController, instead of MasterFactory?
I am not quite sure about this, but it seems to me that
handleException might as well be a package private method in
MasterController.
11. AsynchronousLogShipper, unused import: SQLState
12. AsynchronousLogShipper, constructor: You have removed the
exception from the javadoc, but not the throws list.
13. AsynchronousLogShipper#run:
a) "when the shipper is informed to perform its periodic shipping
also." I feel this sentence is a bit unclear. What does is mean
to be "informed to perform its periodic shipping"?
b) I think the interrupted exception should be caught and ignored.
This will make it possible for someone to immediately stop the
shipping thread in a controlled way by first setting the stop
flag and then interrupting the thread.
14. AsynchronousLogShipper#shipALogChunk/forceFlush: I am not sure
that it is a good idea to call handleException when you are not in
the log shipping thread. In that case, one might as well just let
the exception go up to the MasterController directly. I would
also think it is the master controller that should decide whether
or not to reconnect, not the log shipper on its own. With respect
to NoSuchElementException, I think you have better knowledge of
the context in which it is happening here than the
MasterController will have, and it might be a good idea that the
log shipper decides how to handle it. Since it will occur if you
do not get any data even if next() return true, something fatal
must have happened, and I guess you might as well report back a
fatal replication error.
15. AsynchronousLogShipper#forceFlush: Why use notifyAll() when there
should be only one thread waiting? I would use notify() instead.
16. AsynchronousLogShipper#stopLogShippment: Typo: there should be
only one 'p' in shipment.
> Implement the LogShipper that will enable the shipping of Log records from
> the master to the slave
> --------------------------------------------------------------------------------------------------
>
> Key: DERBY-3064
> URL: https://issues.apache.org/jira/browse/DERBY-3064
> Project: Derby
> Issue Type: Sub-task
> Reporter: V.Narayanan
> Assignee: V.Narayanan
> Attachments: LogShipperImpl_v1.diff, LogShipperImpl_v1.stat,
> LogShipperImpl_v2.diff, LogShipperImpl_v2.stat, LogShipperImpl_v3.diff,
> LogShipperImpl_v3.stat
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
