Derby
  1. Derby
  2. DERBY-2248

Place holder for the NetworkServer system test

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None

      Description

      I will be using this Jira entry as the place holder for the contibution of NetworkServer system tests.

      1. NsTest.zip
        35 kB
        Manjula Kutty
      2. DERBY-2248_stat.txt
        1 kB
        Manjula Kutty
      3. DERBY-2248_diff.txt
        0.6 kB
        Manjula Kutty
      4. build.xml
        3 kB
        Manjula Kutty

        Activity

        Hide
        Jean T. Anderson added a comment -

        The software grant for this contribution was received, it just hasn't been recorded yet. I suggest that Manjula go ahead and upload the code when it's ready so people can start looking at it. It just can't be committed until the ip clearance process is complete (including the software grant recorded).

        Show
        Jean T. Anderson added a comment - The software grant for this contribution was received, it just hasn't been recorded yet. I suggest that Manjula go ahead and upload the code when it's ready so people can start looking at it. It just can't be committed until the ip clearance process is complete (including the software grant recorded).
        Hide
        Manjula Kutty added a comment -

        Adding the source file for review. Thanks Jean for the update.

        Show
        Manjula Kutty added a comment - Adding the source file for review. Thanks Jean for the update.
        Hide
        John H. Embretsen added a comment -

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

        Show
        John H. Embretsen added a comment - 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).
        Hide
        Daniel John Debrunner added a comment -

        This test seems similar in approach to the single table test added under DERBY-2134.

        Do we need two similar tests or could they be combined into a single "single table" test?

        Show
        Daniel John Debrunner added a comment - This test seems similar in approach to the single table test added under DERBY-2134 . Do we need two similar tests or could they be combined into a single "single table" test?
        Hide
        Manjula Kutty added a comment -

        I agree that this test is similar to single table test in some ways. But this test tests more features like triggers ,backup/restore etc.

        Show
        Manjula Kutty added a comment - I agree that this test is similar to single table test in some ways. But this test tests more features like triggers ,backup/restore etc.
        Hide
        Manjula Kutty added a comment -

        Attaching build.xml and the patch for changes in the build.xml under java/testing directory. Please review and commit

        Show
        Manjula Kutty added a comment - Attaching build.xml and the patch for changes in the build.xml under java/testing directory. Please review and commit
        Hide
        Jean T. Anderson added a comment -

        This contribution is cleared for import. Here's the final notice sent to the Incubator:
        http://mail-archives.apache.org/mod_mbox/incubator-general/200703.mbox/%3c45E857D1.6080909@bristowhill.com%3e

        Show
        Jean T. Anderson added a comment - This contribution is cleared for import. Here's the final notice sent to the Incubator: http://mail-archives.apache.org/mod_mbox/incubator-general/200703.mbox/%3c45E857D1.6080909@bristowhill.com%3e
        Hide
        Andrew McIntyre added a comment -

        Committed to trunk with revision 514882.

        Show
        Andrew McIntyre added a comment - Committed to trunk with revision 514882.
        Hide
        John H. Embretsen added a comment -

        I entered a new issue, DERBY-2479, to track the ArrayIndexOutOfBoundsException I mentioned in an earlier comment to this issue, since I have not seen any indications of responses to any of the review comments or questions I posted.

        Show
        John H. Embretsen added a comment - I entered a new issue, DERBY-2479 , to track the ArrayIndexOutOfBoundsException I mentioned in an earlier comment to this issue, since I have not seen any indications of responses to any of the review comments or questions I posted.

          People

          • Assignee:
            Manjula Kutty
            Reporter:
            Manjula Kutty
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development