[
https://issues.apache.org/jira/browse/DERBY-2248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12474445
]
John H. Embretsen commented on DERBY-2248:
------------------------------------------
I downloaded, unzipped, built and ran the test. I also looked at some of the
source code. I have some questions and comments (don't let the amount of text
scare you, it's mostly nitpicking, but I found at least one real bug as well):
General comments:
------------------
- How long (approximately) is the test expected to last with default settings?
The README should include a comment on this.
- Very few of the method and field comments have valid JavaDoc formatting. For
example, the printException( ) method of nstest.utils.DbUtil.java has the
following (rather messy, IMHO) method comment:
// ** This method abstracts exception message printing for all exception
// messages. You may want to change
// ****it if more detailed exception messages are desired.
// ***Method is synchronized so that the output file will contain
sensible
// stack traces that are not
// ****mixed but rather one exception printed at a time
- Some of the Java statements spanning multiple lines are not indented
"properly", see for example line 522-523 of DbUtil.java:
long rowToReturn = (minVal + 1)
+ (Math.abs(rand.nextLong()) % (maxVal - minVal));
This may confuse reviewers, and may increase the possibility of bugs if the
test is modified in the future.
- One possible item to add as "future work" could be to let the server host
and port number be configurable (the client URL is currently hard-coded to
localhost and port 1900).
o.a.d.s.nstest.NsTest.java:
---------------------------
- The CREATE_DATABASE_ONLY field could use a comment at the point of
declaration. (Usage comments below are fine).
- line 217, addStats( ) method: The switch (type) case statements could use the
previously declared fields (static ints) INSERT, UPDATE, DELETE, etc. instead
of using the ints 0, 1, 2 etc. directly, couldn't they? This would improve
readability and maintainability.
- line 264, main( ) method: Why throw both SQLException, IOException,
InterruptedException, Exception, and Throwable? I know it's a good habit to
declare checked Exceptions individually, but just throwing Throwable would
cover all of the above in this case. The JavaDoc should however document all
of the exceptions in @throws clauses.
Having said that, I think it's overkill to throw Exception and Throwable.
For example, the methods in the DbUtil class probably won't throw anything
but SQLExceptions, hardly even that, although it is declared to throw
Exception. You may perhaps be able to avoid throwing anything from the main
method at all if you reconsider which exceptions to throw from the various
methods called from main( )...
- line 429->: while (numTestThread < maxTestThreads) { ...
***BUG***:
Why the while loop here? It is broken...
It seems that numTestThread will always be equal to maxTestThreads after one
iteration, except when derby.nstest.backupRestore=false, in which case
the entire loop will re-run, numTestThread will be incremented far above
maxTestThreads, and you end up with an ArrayIndexOutOfBoundsException:
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 71
at
org.apache.derbyTesting.system.nstest.NsTest.main(NsTest.java:443)
- line 471-490: When printing statistics: The text
"Note that this may not be the same as the server side connections made to the
database especially if connection pooling is employed"
is printed, but the actual number of connections (numConnections) is not
printed.
o.a.d.s.nstest.utils.DbUtil.java:
---------------------------------
- method pick_one( ) (line 490->):
I don't understand the logic of this method. The comments say that a row with
a random key value between minVal and maxVal will be returned. However, from
what I can see, the value being returned is not really random unless the
statement
"select max(serialkey) from nstesttab where serialkey > ?"
(where ? translates to <random value between minVal and maxVal>)
returns a value <= 0. If max(serialkey) is > 0 (as it usually is), the max
will be returned. This is supported by the output from the test program, e.g.:
------<8-------- output excerpt start ---------------<8---------------
Tester2Thread 24 deleted row with serialkey 50638 *** SUCCESS ***
Tester2Thread 17 dbutil.pick_one() -> Obtained row from the table 50636
Tester2Thread 17 attempting to update col t_float to 8.4019195E17
Tester2Thread 17 updated 1 row with serialkey 50636 *** SUCCESS ***
Tester2Thread 17 inserted 1 row with id 1844913544 *** SUCCESS ***
Tester1Thread 1 dbutil.pick_one() -> Obtained row from the table 50639
Tester1Thread 1 attempting to delete a row with serialkey = 50639
Tester1Thread 1 deleted row with serialkey 50639 *** SUCCESS ***
Tester2Thread 53 dbutil.pick_one() -> Obtained row from the table 50636
------<8-------- output excerpt end ---------------<8-----------------
- line 597: println() typo, "exection" should be "exception"
o.a.d.s.nstest.tester.Tester[1,2].java:
---------------------------------------
- startTesting methods: Comments do not match the code...
Comments first say (this is from Tester2.java, line 57):
"Autocommit is left on, so per connection, we make MAX_OPERATIONS_PER_CONN
number of transaction batches"
Later comments say (both Tester1 and Tester2):
//set autocommit to false to keep transaction control in your hand
//Too many deadlocks amd locking issues if this is not commented out
Then, autocommit is actually set to false in both Testers, i.e. it is _not_
commented out, and is thus _not_ left on. Should autocommit be on or off by
default?
I did not study all classes/methods that carefully, so I may have missed a few
things.
Running the test went fine (still running at the time of writing this, has been
running for several hours), apart from the ArrayIndexOutOfBoundsException
mentioned above. The bug(s) should be fixed, but I'm fine with this code being
committed (barring any vetos in the vote on derby-dev).
> Place holder for the NetworkServer system test
> ----------------------------------------------
>
> Key: DERBY-2248
> URL: https://issues.apache.org/jira/browse/DERBY-2248
> Project: Derby
> Issue Type: Test
> Components: Test
> Affects Versions: 10.3.0.0
> Reporter: Manjula Kutty
> Assigned To: Manjula Kutty
> Priority: Trivial
> Fix For: 10.3.0.0
>
> Attachments: NsTest.zip
>
>
> I will be using this Jira entry as the place holder for the contibution of
> NetworkServer system tests.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
