db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel John Debrunner (JIRA)" <derby-...@db.apache.org>
Subject [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Date Sat, 11 Jun 2005 18:44:48 GMT
    [ 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


Mime
View raw message