[
https://issues.apache.org/jira/browse/DERBY-3064?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12526154
]
Jørgen Løland commented on DERBY-3064:
--------------------------------------
Thank you for addressing this issue, Narayanan.
I had a look at the preliminary patch and have some questions.
1. I think "Factory" is used for object creation in cases where the object
creation work is hidden from the caller. This is the case for bootable services
since the boot method does this masking. In the log shipping case, however, I
don't think "Factory" is a good name. Maybe you could just call the interface
(Replication)LogShipper?
2. The package names are very long. Since the package already contains
"replication.master", maybe we could shave off some characters in the class
names? "Replication" could, e.g., be removed. Just a thought.
3. Since you already have commented the variables, I think you should add
another * (/* -> /**) so that the comments become javadoc
4. I think the Daemon should be created by the MasterController since that
class will be managing everything on the master side
5. AsynchLogShipper does not need to know about the transmitter since Forced...
does the log shipping. The same goes for the log buffer.
6. How do you intend to create the log shipping service? I see that Forced...
implements the interface, but that the creator of Asynch... creates a Forced...
object. In MasterController, we would want to create an instance of the
interface. Maybe Asynch could extend Forced? Again, just a thought. With that
design, all replication strategy specific methods like Forced#flushedInstance
could be made abstract, and adding other replication strategies (like 2-safe)
later would be really simple.
7. The javadoc for forceFlush is not describing the behavior of the method
8. I am concerned about the eternal loop in performWork. I guess this will be
way too active, especially in low workload cases. Also, control should be
returned from the performWork method. It should be up to the DaemonService to
determine when it wants to ship log. A slight improvement could be to let
shipALogChunk returned the boolean return from LogBuffer#next, and change
performWork to look something like this:
performWork() {
boolean sentSomething = true;
while (sentSomething) {
sentSomething = forcedLogShipper.shipALogChunk();
}
}
or even
performWork() {
if (currentTimeMillis >= (timeOfLastLogShipment + shipmentWaitTime) {
boolean sentSomething = true;
while (sentSomething) {
sentSomething = forcedLogShipper.shipALogChunk();
}
}
}
> 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
> Attachments: LogShipperImpl_v1.diff, LogShipperImpl_v1.stat
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
