db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John H. Embretsen (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-2248) Place holder for the NetworkServer system test
Date Tue, 20 Feb 2007 14:01:08 GMT

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


Mime
View raw message