On 2/12/06, Bryan Pendleton <[EMAIL PROTECTED]> wrote: > 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.
Thanks Bryan for reviewing the patch. > 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. I was planning to submit 8,9,10 bullets in my description as one patch. But it seems better to separate 10 as a separate patch. Then I plan to submit 1,3,4 as one patch. And after that the patch to enable the test derbyStress.java to run with client driver. > - 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. I have removed reset of "needsToSendParamData". I have kept reset of currentDrdaRs in DRDAStatement.close() itself since close() method resets variables in DRDAStatement. I was trying to see if I can avoid doing a "new DRDAResultSet()" in the close method since it looks a little odd to create a new object in close method. I guess that is fine since close() method is also called when re-using the statement. > - 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(). I have renamed the method to markClosed(). As you pointed out, cleanupParameters is the equivalent of 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. > I have added more comments. Please go through them and let me know if I have missed something. I will attach the patches to JIRA in a while. Thanks, Deepa
