db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Øystein Grøvlen (JIRA) <j...@apache.org>
Subject [jira] Commented: (DERBY-2921) Replication: Add a network service that connects the master and slave Derby instances
Date Mon, 03 Sep 2007 18:32:58 GMT

    [ 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.


Mime
View raw message