db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ole Solberg (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-3162) Create a framework for replication tests
Date Fri, 29 Feb 2008 14:32:51 GMT

    [ https://issues.apache.org/jira/browse/DERBY-3162?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12573764#action_12573764
] 

Ole Solberg commented on DERBY-3162:
------------------------------------

Thanks Øystein for reviewing this!

> Øystein Grøvlen commented on DERBY-3162:
> ----------------------------------------
> 
> I tried to run the ReplicationRun_Local test, but in order for it to
> run, I had to manually create the directories db_master and db_slave
> for the test to run.

I missed that one, probably because I have been running in the same dirs all the time so it
was always there...

> 
> Since this patch does not affect people not working on replication, I
> suggest that I commit the patch more or less as it is, and then we can
> do I more thorough review afterwards.

I would appreciate that. It will probably make it easier to incrementally do all the required
enhancements and fixes pointed out below.

> 
> Some initial comments based on a quick look at parts of the patch:
> 
>  - I assume you did not intend to include the build.xml.orig file.

Can't find that in my sandbox!

> 
>  - Do you intend to document ReplicationRun_Local in
>    README.runningTests?

I will.

> 
>  - I did not understand the following line in README.runningTests:
>       cd <testing dir>/ # ${user.dir}
I'll clean that up (I probably meant that ${user.dir} would be <testing dir>.)

> 
>  - README.framework seems a bit out of date

Will fix.

> 
>  - Some general observations:
>     - Several classes has a lot of unused imports.
>     - Many very long lines
>     - For some classes I miss an initial javadoc that explains the
>       purpose of the class.
>     - As far as I know, there should be only one exception per @throws
>       tag.
> 



> Øystein Grøvlen commented on DERBY-3162:
> ----------------------------------------
> 
> I have looked a bit closer at the ReplicationRun class, and I have
> some comments.  Note, however, as I said in my previous comment, if
> you like, I can commit the existing patch and you can work on the
> improvements in follow-up patches.  Some comments:
> 
>  - I must admit that it took me some time to understand how I should
>    go about to add new tests, but if I understand it correctly, the
>    way to do it is to subclass ReplicationRun for my test class and
>    use the methods of ReplicationRun to initiate replication.  I miss
>    some instructions on how to go about writing new tests in this
>    framework.

Yes, that is the intention. I agree instructions should be enhanced.

> 
>  - I feel it is a bit confusing that ReplicationRun seems to be both a
>    framework and a test.  I think it would be better if the methods
>    used to perform replication operations were separated from actual
>    test cases.

The test part should be removed.

> 
>  - I also think it would be a good idea to separate the code for
>    testing with localhost from the code for testing with master and
>    slaves on different hosts.  In other words, I suggest having two
>    classes (with a common base class), one for local testing and one
>    for distributed testing both implementing the same set of methods
>    like startServer, initMaster etc.

Yes, when looking at this now I tend to agree. I started out creating the framework (and test)
for the distributed case and the "localhost case" was added later.

> 
>  - I miss some documentation of all the static fields in
>    ReplicationRun.  

Will add.

> 
>  - Many of the fields are both initialized when they are declared and
>    in initEnvironment.  It seems a bit errorprone to duplicate this.

Thanks for pointing this out - I have stared too long at this code alone I guess ;-)
> 
>  - Why is masterServerHost etc. set to localhost in ReplicationRun?  I
>    thought that was supposed to be behavior that were specific to
>    ReplicationRun_Local.

Ditto.

> 
>  - I think the classes Utils and State should be given more specific
>    names.

Will fix.

> 
>  - I think it is a source of confusion that both ReplicationRun and
>    its inner class State contains a method called initEnvironment.  By
>    the way, the State does not currently seem to be in use.

Will clean up this.

> 
>  - Some unused imports: FileChannel, Inet6Address, JDBC
> 
>  - I am not sure it is a good idea to import whole packages.  At
>    least for java.util, Properties seems to be the only class used.
>    Why not import just that class.

Thanks for pointing this out.

> 
>  - The freezeDB string is empty.  Note that you still need to freeze
>    the database when replication is started.

Will clean up.

> 
>  - Some comments have references to PoC.  I guess that is obsolete.

Will clean up.

> 
>  - Does masterServer and slaveServer need to be declared at class
>    level?  They seem to be local to the test case when they are used.

Thanks for pointing this out.

> 
>  - Why implement setUp and tearDown when they only call the
>    corresponding method in the super class?

Thanks for pointing this out.

> 
>  - The file name README.properties is a bit awkward since IDEs and
>    other tools may think it is a properties file, which it is not.

Thanks! did not think of that!

> 




> Create a framework for replication tests
> ----------------------------------------
>
>                 Key: DERBY-3162
>                 URL: https://issues.apache.org/jira/browse/DERBY-3162
>             Project: Derby
>          Issue Type: Sub-task
>          Components: Test
>    Affects Versions: 10.4.0.0
>            Reporter: Ole Solberg
>            Assignee: Ole Solberg
>            Priority: Minor
>         Attachments: derby-3162.1-v1.diff.txt, derby-3162.2-v2.diff.txt, derby-3162.2-v2.stat.txt
>
>
> Handle
>  - starting and stopping Derby servers to have the master and slave replication roles,
>  - doing administrative commands like startreplication, startslave, stopreplication,
failover,
>  - performing consistency checks on the slave vs. the master,
>  - running load clients against master and slave in the various states of replication,
>  - provoking error situations on master and slave, and network,
>  - ... 

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