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