[
https://issues.apache.org/jira/browse/DERBY-2921?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12524568
]
Øystein Grøvlen commented on DERBY-2921:
----------------------------------------
Narayanan, thanks for the patch. Here are my comments:
1. ReplicationMessageTransmit and ReplicationMessageReceive has much
in common, but the organization of the code differs in several
places. Examples:
- The ...Transmit constructor calls a private function which
does the necessary work while ...Receive has much of the code
in the constructor.
- ...Tramsmit has a separate class for the privileged operation,
...Receive does not.
I think it would be better if the code was more uniform. Maybe
some of it could even be shared? (E.g., member variables and code
to resolve defaults.)
2. Only part of the code in the ReplicationMessage... classes is
particular to replication. For example,
ReplicationMessageTransmit#sendAndReceiveAck is the only method in
that class that has any replication specific code. I think you
should consider to factor out the replication specific code to
separate classes that use your Transmit and Receive classes.
3. It is not clear to me what the user contract is for the
ReplicationMessage... classes. I think you need to add Javadoc
that describe how they are supposed to be used, and how they behave
in the normal case (e.g., it needs to be mentioned that
ReplicationMessageReceive#receive does not return until an error
occurs) and in a failure situation (e.g., will they try to
reconnect automatically?).
4. I do not think you need ReplicationMessageTransmit#s. The socket
is only referred directly in getSocketAndInputOutputStreams() and
might as well be local to that method.
5. I think it would be good with more meaningful names for oos and
ois. Such names are ok for local variables, but for class members
I think you should use something more self-explanatory.
6. The check for default values does not have to be done on every
connect so I suggest it can be moved to the constructor.
7. ReplicationMessageTransmit#getSocketAndInputOutputStreams has two
occurences of comments where only one of ObjectOutputStream and
ObjectInputStream is mentioned, but I think both should. (javadoc
and last comment).
8. ReplicationMessageTransmit#send: Local variable 'answer' is not
needed.
9. ReplicationMessageTransmit#send: Last sentence of comment is a bit
misleading. This code does not throw an exception if a connection
is not established. (However, I guess the underlying code will
throw such an exception)
10. ReplicationMessageReceive constructor takes a host address, and
not a host name. This means that the caller need to do the
mapping between hostname and host address. Would it not be better
to do this mapping within the class. That would also make the
interfaces of ...Transmit and ...Receive more similar.
11. ReplicationMessageReceive#createServerSocket tests whether
hostname is null. I think it should be hostAddress.
12. Why is ReplicationMessageReceive#blockingRead public? Can one
call this directly instead of going through #receive?
13. ReplicationMessageReceive#parseInitiatorMessage: Why check the
version here? It seems to already have been done in
ReplicationMessage#readExternal.
14. ReplicationMessageReceive javadoc: "receives the message from
the slave ...". Is this class only to be used by the master?
15. ReplicationMessage javadoc: Should say that this is for
replication.
16. ReplicationMessage: I think it needs to be explicitly defined what
kind of object the different types of messages will take. (e.g.,
TYPE_LOG is followed by a log record, TYPE_INITIATE is followed
by a string)
17. Have you considered to use SQL states instead of general text for
TYPE_ERROR messages?
18. I think it would be good with a unit test that tests the basic
capabilities of this service.
19. Nit: The code is a bit inconsistent with respect to spaces before
left paranthesis. Most Derby code does not use space after
function names in declarations and calls, while space is used
after reserved words like if, while, for etc.
> Replication: Add a network service that connects the master and slave Derby
> instances
> -------------------------------------------------------------------------------------
>
> Key: DERBY-2921
> URL: https://issues.apache.org/jira/browse/DERBY-2921
> Project: Derby
> Issue Type: Sub-task
> Components: Services
> Affects Versions: 10.4.0.0
> Reporter: Jørgen Løland
> Assignee: V.Narayanan
> Attachments: Replication_Network_v1.diff,
> Replication_Network_v1.stat, Replication_Network_v2.diff,
> Replication_Network_v2.stat, Replication_Network_v3.diff,
> Replication_Network_v3.stat
>
>
> A network connection is required between the master and slave Derby instances
> of a replicated database. The connection will be used to send many kinds of
> messages, including:
> * log records
> * the database (when replication is started)
> * master -> slave commands (like "stop replication")
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
