[
https://issues.apache.org/jira/browse/DERBY-2977?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12530954
]
Øystein Grøvlen commented on DERBY-2977:
----------------------------------------
The patch looks very good. I have a few comments, but most are minor nits:
1. LogToFile:
It seems a bit strange to add checks for replicationMasterMode in
situations where replication should not be going on (e.g., during
recovery). Would it be better to add a method to LogToFile that
the LogAccessFile constructor could use to check whether
replication was going on? Or maybe pass it as a parameter to the
constructor? With the current usage, I would change the name of
LogAccessFile#startReplicationMasterRole since it is not called
only when replication is started.
2. ReplicationLogScan:
I am not quite sure I like the way you handle the mix of log
records and log file switch records. I wonder whether it would be
cleaner if when a log file switch is encountered, one would set the
logFileSwitch flag and continue to read the next log record. Then,
if next() returns true there will always be a valid log record, but
you will have to check for log file switch before you handle it.
3. ReplicationLogScan, javadoc:
Why does the @see list the same class twice?
4. ReplicationLogScan#next, comment in exception handling (not new):
Typo: (currupt)
5. ReplicationLogBuffer, javadoc (Not added by this patch):
I feel "... consists of n LogBufferElement, each of which can store
m log records ..." is a bit confusing since it sounds like the
number of log records in a buffer (m) is a configurable constant.
6. ReplicationLogBuffer#appendBytes:
You do not want javadoc for the params?
7. MasterController:
Import of ReplicationMessage is unused.
8. MasterController/MasterFactory#appendLogRecord, javadoc:
Mismatch between javadoc and parameter name for length.
9. MasterController#startReplicationMasterRole/stopReplicationMasterRole:
Why is one of the member fields prefixed with 'this.', but not the
other?
> Replication: Add a ReplicationMaster controller that will manage replication
> on the master side
> -----------------------------------------------------------------------------------------------
>
> Key: DERBY-2977
> URL: https://issues.apache.org/jira/browse/DERBY-2977
> Project: Derby
> Issue Type: Sub-task
> Components: Services
> Affects Versions: 10.4.0.0
> Reporter: Jørgen Løland
> Assignee: Jørgen Løland
> Attachments: derby_2977_1.diff, derby_2977_1.stat,
> derby_2977_1b.diff, derby_2977_1b.stat, derby_2977_1c.diff,
> derby_2977_1c.stat, repli_logbuffer_v2.diff, repli_logbuffer_v2.stat
>
>
> The replication master role includes many tasks:
> * set up a network connection to the slave
> * sending the database to the slave before starting replication
> * make sure that log records are appended to the network buffer, and that the
> log is later sent to the slave
> * etc
> This issue is for adding a controller that will start/stop/initiate all
> services needed for the replication master role.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
