db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bryan Pendleton <bpendle...@amberpoint.com>
Subject Re: [jira] Updated: (DERBY-1002) Check that DRDAStatement and DRDAResultSet states are reset when they are re-used
Date Sun, 26 Feb 2006 19:16:26 GMT
Hi Deepa,

Thanks for pulling this patch together. This is excellent work,
and I think the new structure of initialize/close/reset/setTypDefValues
is much cleaner and easier to read.

I successfully applied your patches, and I can confirm that your
new regression test provokes the expected failure(s) without the
patch, and succeeds with the patch.

Three small comments:

1) It wasn't clear to me why these needed to be applied as two
separate patches. I would be comfortable putting the test and the
fix in the same commit; is there something I'm missing here?

2) There's a mysterious comment in DRDAStatement.initialize():

   +	 * TODO: Need to see what exactly it means to initialize the default
   +	 * statement. (DERBY-1002)

Does this mean that there is still a loose end to tie up here?

3) It took me a little while to realize that the idea of testStatementReuse
is that it sets up the section tables in such a way as to provoke statement
re-use in tests jira_491_492 and testImplicitClose. That is, testStatementReuse
isn't directly testing the problem, but rather is setting things up so that
the other two tests will see a "more interesting" environment.

It seems like it would be nice to be more explicit about this technique
in the comments at least, because it's kind of subtle and when I first read
the new test code, I thought that you had omitted something because the
end of testStatementReuse() seems to trail off without testing anything;
moreover, it refers to something called cSt2 which doesn't actually exist in
that test case:

+		// Close cSt1 so that cSt2 will reuse same DRDAStatement
+		cSt1.close();

My suggestion is:
  - fix that last comment in testStatementReuse to make it clear that we are
    arranging to have a re-usable statement be "ready" at the end of this
  - fix the method-level comments for testStatementReuse so that instead of

    +     * This test creates statements and closes them to provoke the network server to
    +     * re-use statements to test that re-use happens correctly. It gives error without
    +     * patch for DERBY-1002.

    they instead make it clear that this test sets up a statement which is ready to be
    re-used, so that the *next* test will start by reusing an old statement.
  - add a one-line comment at the two places (in testImplictClose() and in
    jira_491_492()) where you have planted the call to testStatementReuse() saying
    something like:

    // Cause an existing statement to be opened and closed so that it will be re-used

Hope this is helpful,


View raw message