[ http://issues.apache.org/jira/browse/DERBY-31?page=comments#action_12313375 ]
Daniel John Debrunner commented on DERBY-31: -------------------------------------------- Some general comments on the patch - giving the patch file a specific name (rather than svn-diff) helps out the committers (and anyone else) when the patch is downloaded - try to resist the temptation to clean up the code (e.g. your javadoc fixes), it makes it much hard to review the patch and see the real changes. If you want to do such cleanup that's great, by a separate patch is much better. Specific comments - the patch needs to be re-submitted, it doesn't apply for me on messages_en.properties and SQLState, just update your codeline and see if a merge is needed. In general I believe the functionality is correct and clearly implemented but I have a few minor concerns and one major one. The major one is the performance impact of this fix . Every time StatementContext.setInUse() is called a new CancelQueryTask object will be created, that will be very expensive for queries, imagine a ResultSet of one milllion rows, that's at least one million short lived objects being created and going through gc. Derby tries to limit object creation during execution to be not be a function of the number of rows, except where (indirectly) mandated by the JDBC spec. The impact of the checkCancellationFlag() calls is also of concern. - I wonder if a module is really needed for the time task, a simpler approach would be just to have the Monitor return the TimerTask directly. The TimerFactory interface doesn't add any value over TimerTask itself, and its one method ie badly named. I don't see any reason this timer should be limited to cancellation events. - The code seems to vary a bit about choosing to use milli-seconds or seconds, it has to be seconds at the JDBC level, but then internally you use milli-seconds and then back to seconds in the langauage PreparedStatement interface. I would stick to seconds since that is the granuality at the api level and only convert to ms at the TimerTask level. You might consider using the constant 0L if you are passing zero into a long method argument, means the method resolution remains the same if ever the method is overloaded with an int argument. - For the JDBC Statement.setQueryTimeout() Derby traditionally checks that the input value is valid, thus I think an exception should be thrown for a negative timeout. (and that's the specified JDBC behaviour) In spite of those comments (I'm very picky) I believe this is a good patch, especially for your first. :-) In the open source world it's probably a 50-50 call as to if the patch could be committed as-is and these items subsequently addressed or they need to be addressed first. Some folks on the list may be concerned about the performance impact of this. I would like to see the functionality committed but the performance addressed before this becomes part of any release. [but the patch can only be committed there was a newer version that applied cleanly] It's actually great to have a concrete implementation because the issues with it give me some ideas on how it possibly could be done without such a performance impact, but that will have to be a later e-mail. I need to run :-) > Statement.setQueryTimeout() support. > ------------------------------------ > > Key: DERBY-31 > URL: http://issues.apache.org/jira/browse/DERBY-31 > Project: Derby > Type: New Feature > Components: JDBC > Environment: ALL > Reporter: Ali Demir > Assignee: Oyvind Bakksjo > Attachments: Derby-31.patch, QueryTimer.java, svn.diff, svn.status > > Calling Statement.setQueryTimeout() throws exception saying that function is > not supported. This is an important JDBC feature and is limiting our options > to use Derby with our JDBC code. Implementing this JDBC function would make > Derby much easier to adopt. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
