Deepa Remesh (JIRA) wrote:
I am attaching a draft patch 'derby-210-v2-draft.diff' for review.
Hi Deepa,
I studied this patch for a while, focusing specifically on your bullet
points 8, 9, and 10. The changes look good to me, and I think you are
definitely on the right track.
I had just a few ideas/requests that I wanted to pass along:
- If you're still looking for ways to break the big patch into smaller
ones, change #10 seems to me like it should be separable.
- Regarding change #10, it seems like "needsToSendParamData" is reset
to false by rsClose(), which is called by close(). So maybe you don't
need to reset it to false again in close(). In some ways, it
seems like a simpler way to code change #10 would be to add
currentDrdaRs = new DRDAResultSet();
to DRDAStatement.rsClose(), rather than doing it in DRDAStatement.close().
But I think the way you've done it is fine, too.
- In changes #8 and #9, it seems like it would be more consistent to name
the new PreparedStatement method "markClosed" rather than "cleanupParameters".
Since this markClosed() method would override the one in Statement, you'd
need to be sure to call super.markClosed() during the method. It seems like,
in addition to being more consistent with the superclass, this would also
arrange for the new cleanup code to be called in a few more cases, such as
when methods in the Connection class call Statement.markClosed().
- Lastly, it seems like one of the big parts of your change is to identify
a few simple rules about which methods are safe to call from the finalizers,
and which are not; namely, that finalizer-triggered code is purely local
whereas close code may send messages to the server to call close code there.
I think it would be good to add some comments to the Statement.java and
PreparedStatement.java classes to record this analysis, so that future
readers understand the distinction between the close() and markClosed()
methods.
This is great work you're doing here and I feel like we're in the final stretch
of a big improvement in the client behavior. Thanks much for all the effort!
bryan
