[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ http://issues.apache.org/jira/browse/DERBY-31?page=comments#action_12363737 ] Satheesh Bandaram commented on DERBY-31: Is this issue ready to be RESOLVED and/or CLOSED? Oyvind seems to have done pretty much all activities that need to be done for this issue. 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 Fix For: 10.2.0.0 Attachments: DERBY-31.svn.diff, DERBY-31.svn.status, derbyall_report.txt 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
[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ http://issues.apache.org/jira/browse/DERBY-31?page=comments#action_12357683 ] Oyvind Bakksjo commented on DERBY-31: - Updated documentation which stated setQueryTimeout was not implemented. Sendingsrc/ref/rrefjdbc40794.dita Transmitting file data . Committed revision 344345. 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 Fix For: 10.2.0.0 Attachments: DERBY-31.svn.diff, DERBY-31.svn.status, derbyall_report.txt 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
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Daniel John Debrunner wrote: Daniel John Debrunner wrote: Daniel John Debrunner wrote: I had to re-run the tests because the patch messed up on one file, but now StmtCloseFunTest fails for me with this patch. *** Start: StmtCloseFunTest jdk1.4.2 derbyall:jdbc20 2005-06-28 22:08:57 *** 2a3 Statement Test failed (10) 4a6 Prepared Statement Test failed 7a10 Callable Statement Test failed Test Failed. *** End: StmtCloseFunTest jdk1.4.2 derbyall:jdbc20 2005-06-28 22:09:05 *** I'll look into it, but is anyone else seeing this failure? This is because the setQueryTimeout used to throw a not implemented exception, but now succeeds. But in this case the statement is closed so according to the JDBC spec, all such methods should throw an exception. Oyvind, do you want me to commit your current patch (I've made the copyright date changes, and performed the svn adds and propsets) and then you could fix this problem quickly? I wouldn't want to check in anything that is known to cause tests to fail, even temporarily. The fix is very simple; adding a call to checkStatus() at the very top of EmbedStatement.setQueryTimeout() fixes the problem. If you want me to, I can submit a new patch, but I think the quickest approach would be if you just inserted the 'checkStatus()' call in your own sandbox. Sorry for not catching this one myself; there was a lot of complaining about broken builds and derbyall not running cleanly at the time, and I guess I was just a bit lazy... Won't *ever* happen again. ;o) -- Øyvind Bakksjø Sun Microsystems, Web Services, Database Technology Group Haakon VII gt. 7b, N-7485 Trondheim, Norway Tel: x43419 / +47 73842119, Fax: +47 73842101
[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ http://issues.apache.org/jira/browse/DERBY-31?page=comments#action_12314795 ] Daniel John Debrunner commented on DERBY-31: Øyvind's patch committed revision 208650. (into trunk) 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.svn.diff, DERBY-31.svn.status, Derby-31.patch, QueryTimer.java, derbyall_report.txt 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
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Daniel John Debrunner wrote: [EMAIL PROTECTED] wrote: [on comments about re-use of TimerTasks] write a comment about this in the code in a subsequent patch (for instance when implementing Statement.cancel), unless anybody wants me to address this now and submit a new patch. I think delaying the comment is fine. I'm planning to commit this change before the end of this week, assuming all the tests run for me. I had to re-run the tests because the patch messed up on one file, but now StmtCloseFunTest fails for me with this patch. *** Start: StmtCloseFunTest jdk1.4.2 derbyall:jdbc20 2005-06-28 22:08:57 *** 2a3 Statement Test failed (10) 4a6 Prepared Statement Test failed 7a10 Callable Statement Test failed Test Failed. *** End: StmtCloseFunTest jdk1.4.2 derbyall:jdbc20 2005-06-28 22:09:05 *** I'll look into it, but is anyone else seeing this failure? Dan.
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Daniel John Debrunner wrote: Daniel John Debrunner wrote: [EMAIL PROTECTED] wrote: [on comments about re-use of TimerTasks] write a comment about this in the code in a subsequent patch (for instance when implementing Statement.cancel), unless anybody wants me to address this now and submit a new patch. I think delaying the comment is fine. I'm planning to commit this change before the end of this week, assuming all the tests run for me. I had to re-run the tests because the patch messed up on one file, but now StmtCloseFunTest fails for me with this patch. *** Start: StmtCloseFunTest jdk1.4.2 derbyall:jdbc20 2005-06-28 22:08:57 *** 2a3 Statement Test failed (10) 4a6 Prepared Statement Test failed 7a10 Callable Statement Test failed Test Failed. *** End: StmtCloseFunTest jdk1.4.2 derbyall:jdbc20 2005-06-28 22:09:05 *** I'll look into it, but is anyone else seeing this failure? This is because the setQueryTimeout used to throw a not implemented exception, but now succeeds. But in this case the statement is closed so according to the JDBC spec, all such methods should throw an exception. Oyvind, do you want me to commit your current patch (I've made the copyright date changes, and performed the svn adds and propsets) and then you could fix this problem quickly? I see now you did say [comment from Derby-31] Derbyall has been run with two failures, report attached. StmtCloseFunTest - hard to tell if it's related to my changes, very little output from the test about what exactly failed. The way to investigate this would be to first look at the diff and see if the output helps in any way. In this case there was a new line of Statement Test failed (10) Looking at the test source code, you can then see that the 10 is really a test case number, this was determined by either searching for 10 in the source or searching for the complete text 'Statement Test failed (10)'. Looking at the code for that test case, you can see that the output is printed if setQueryTimeout succeeds. The fact that this is a call to setQueryTimeout means that it is related to your change. Other ways to see which area of a java test is causing the problem is to see if there is a stack trace being printed. If so the line number within the test case is a good starting point. Often the stack trace will be in the '.tmp' output file, but not the '.out' file. Thus it is a good practice in test code to always print the stack trace for any unexpected exception. Dan.
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Daniel John Debrunner wrote: I think delaying the comment is fine. I'm planning to commit this change before the end of this week, assuming all the tests run for me. Great! One thing I do need is confirmation that the copyright dates are correct in the files you've added. Some of them have dates of 1997,2004 which seems unlikely, are all the new files meant to have a copyright date of 2005? I can fix this if it is the case. Yes, 2005 is correct. I'm assuming that some performance tests will be run before this code makes it as part of a release, comparing performance to 10.1/10.0 and the performance impact of enabling query timeout. Yes, I certainly plan to do performance testing of this. rather than a new error XJ074.S, the existing generic error XJ081.S (added by Shreyas) could have been used. OK. Considering e.g. these existing codes (see below), it was not clear to me what was the preferred way - adding a separate code or using a generic one. String INVALID_FETCH_SIZE = XJ062.S; String INVALID_MAX_ROWS_VALUE = XJ063.S; String INVALID_FETCH_DIRECTION = XJ064.S; String INVALID_ST_FETCH_SIZE = XJ065.S; String INVALID_MAXFIELD_SIZE = XJ066.S; -- Øyvind Bakksjø Sun Microsystems, Web Services, Database Technology Group Haakon VII gt. 7b, N-7485 Trondheim, Norway Tel: x43419 / +47 73842119, Fax: +47 73842101
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[EMAIL PROTECTED] wrote: Daniel John Debrunner wrote: rather than a new error XJ074.S, the existing generic error XJ081.S (added by Shreyas) could have been used. OK. Considering e.g. these existing codes (see below), it was not clear to me what was the preferred way - adding a separate code or using a generic one. String INVALID_FETCH_SIZE = XJ062.S; String INVALID_MAX_ROWS_VALUE = XJ063.S; String INVALID_FETCH_DIRECTION = XJ064.S; String INVALID_ST_FETCH_SIZE = XJ065.S; String INVALID_MAXFIELD_SIZE = XJ066.S; I think the discussion was that when Shreyas added a new similar message and ita was resolved that adding a generic message and that existing code could converge to its use over time. I can add a Jira entry for this. My itch is keeping the footprint down in whatever way possible. Dan.
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Hi, Dan. Can you clarify me the motivation behind keeping the footprint down? Are we targeting PDAs? What kind of platforms are you envisioning for Derby that require a minimal footprint? I agree that in general smaller is better, but for instance is smaller a priority over readability/maintainability (for example, building an architecture with fewer classes doing more things rather than more classes with well-defined responsibilities)? Is it a priority over performance? Is it a priority over more advanced features such as replication, XML support, additional SQL features, etc.? What's the order of precedence for these types of key qualities of Derby, in your view? Thanks, David Daniel John Debrunner wrote: [EMAIL PROTECTED] wrote: Daniel John Debrunner wrote: rather than a new error XJ074.S, the existing generic error XJ081.S (added by Shreyas) could have been used. OK. Considering e.g. these existing codes (see below), it was not clear to me what was the preferred way - adding a separate code or using a generic one. String INVALID_FETCH_SIZE = XJ062.S; String INVALID_MAX_ROWS_VALUE = XJ063.S; String INVALID_FETCH_DIRECTION = XJ064.S; String INVALID_ST_FETCH_SIZE = XJ065.S; String INVALID_MAXFIELD_SIZE = XJ066.S; I think the discussion was that when Shreyas added a new similar message and ita was resolved that adding a generic message and that existing code could converge to its use over time. I can add a Jira entry for this. My itch is keeping the footprint down in whatever way possible. Dan.
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Daniel John Debrunner wrote: Oyvind Bakksjo (JIRA) wrote: Hi Dan, thanks for reviewing this patch so quickly. I am fully aware of and share you concern for the performance impact of creating all the TimerTask objects. It is far from ideal. The problem is that the Timer class _forces_ recreation of TimerTask objects, since these can't be reused; when a TimerTask has run or been cancelled, it is pure waste. Trying to schedule the same task again will cause an exception. If it wasn't for this, we could associate a TimerTask with each StatementContext object. This is the kind of case where a comment in the code helps a great deal. A comment that says why a new TimerTask is created each time due to the requirements of the Java timer scheme. Not everyone will know or remember restrictions on TimerTasks. Ane when I looked at the JDK 1.4 javadoc it wasn't clear to me that that was the intention of the scheme. The implication TimerTasks can not be re-used is hidden the exception description for adding one to Timer. I would almost read the javadoc as you can re-use timer tasks, but a timer task can not be scheduled multiple times concurrently. No, scheduling an already used TimerTask will generate an IllegalStateException if any of the following are true (I have verified this): a) the task has been scheduled but has not yet been run b) the task has been scheduled and has been run c) the task has been scheduled and has been cancelled In other words, TimerTask objects can be scheduled only once, they can not be reused. I suggest that I write a comment about this in the code in a subsequent patch (for instance when implementing Statement.cancel), unless anybody wants me to address this now and submit a new patch. -- Øyvind Bakksjø Sun Microsystems, Web Services, Database Technology Group Haakon VII gt. 7b, N-7485 Trondheim, Norway Tel: x43419 / +47 73842119, Fax: +47 73842101
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[EMAIL PROTECTED] wrote: [on comments about re-use of TimerTasks] write a comment about this in the code in a subsequent patch (for instance when implementing Statement.cancel), unless anybody wants me to address this now and submit a new patch. I think delaying the comment is fine. I'm planning to commit this change before the end of this week, assuming all the tests run for me. One thing I do need is confirmation that the copyright dates are correct in the files you've added. Some of them have dates of 1997,2004 which seems unlikely, are all the new files meant to have a copyright date of 2005? I can fix this if it is the case. I'm assuming that some performance tests will be run before this code makes it as part of a release, comparing performance to 10.1/10.0 and the performance impact of enabling query timeout. I'm also guessing there are other improvements that could be done, such as tying the lock timeout to the query timeout value. Two minor comments on the patch rather than a new error XJ074.S, the existing generic error XJ081.S (added by Shreyas) could have been used. EmbeddedStatement now calculates query timeout * 1000 for every execution, probably minor but might have been better to store the value as ms in EmbeddedStatement and just divide / 1000 when getQueryTimeout is called. Seems execute will be called far more often than getQueryTimeout. Thanks! Dan.
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Oyvind Bakksjo (JIRA) wrote: Hi Dan, thanks for reviewing this patch so quickly. I am fully aware of and share you concern for the performance impact of creating all the TimerTask objects. It is far from ideal. The problem is that the Timer class _forces_ recreation of TimerTask objects, since these can't be reused; when a TimerTask has run or been cancelled, it is pure waste. Trying to schedule the same task again will cause an exception. If it wasn't for this, we could associate a TimerTask with each StatementContext object. This is the kind of case where a comment in the code helps a great deal. A comment that says why a new TimerTask is created each time due to the requirements of the Java timer scheme. Not everyone will know or remember restrictions on TimerTasks. Ane when I looked at the JDK 1.4 javadoc it wasn't clear to me that that was the intention of the scheme. The implication TimerTasks can not be re-used is hidden the exception description for adding one to Timer. I would almost read the javadoc as you can re-use timer tasks, but a timer task can not be scheduled multiple times concurrently. Dan.
[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ http://issues.apache.org/jira/browse/DERBY-31?page=comments#action_12313431 ] Oyvind Bakksjo commented on DERBY-31: - Hi Dan, thanks for reviewing this patch so quickly. I am fully aware of and share you concern for the performance impact of creating all the TimerTask objects. It is far from ideal. The problem is that the Timer class _forces_ recreation of TimerTask objects, since these can't be reused; when a TimerTask has run or been cancelled, it is pure waste. Trying to schedule the same task again will cause an exception. If it wasn't for this, we could associate a TimerTask with each StatementContext object. This performance impact will, however, only affect queries with a timeout value set. As such, it is a tradeoff - you can get timeouts, but that will slow down your execution. If it turns out that the calls to checkCancellationFlag() seriously affect performance, these calls could be decimated by doing the call only for each n-th row. (It sure will be nice to get that performance regression test, so we'll see the actual impact.) For the time being, I can do some rudimentary testing of this myself. I agree a separate module for the timer might be overkill. As for the interface, my idea was that more methods than getCancellationTimer() could be added as needs arose for Timers for other purposes. It would be up to the implementing class whether to actually return the same Timer object. I guess it would depend on the duration of the TimerTasks and the responsiveness requirements whether to use multiple Timers or just a single one. I meant to support milliseconds use internally and seconds at the JDBC API level. I guess it would be more consistent if I used milliseconds in the language PreparedStatement interface as well. And yes, I'll fix throwing an exception on negative timeout values. 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
[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ 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
Re: [jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
Oyvind Bakksjo (JIRA) wrote: Here is an implementation of Statement.setQueryTimeout. The patch includes a new functional test in the jdbcapi testsuite. It is my first Derby patch, so I ask any reviewer; please, use your magnifying glass and be critical. :) May I suggest that Dan looks at it, since he has given me the most feedback on this issue? (I don't write this to exclude other volunteers, I just think that Mr. Someone already has enough on his todo list, given all the requests of the kind Can someone review this patch?. ;o) I will look at it over this weekend. I think it missed the train for 10.1, but it could potentially go into a future 10.1 release (ie. on the branch) because it (I assume) is just runtime code, no upgrade or persistent issues. Dan.
[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ http://nagoya.apache.org/jira/browse/DERBY-31?page=comments#action_56812 ] Amit Handa commented on DERBY-31: - One solution can be to check for time(set a flag) when setQueryTimeout() is set. Every time any executeXXX() method is called, check whether query timed out with differrence from the present time and the flag set earlier. If time out throw SQLException else do nothing Statement.setQueryTimeout() support. Key: DERBY-31 URL: http://nagoya.apache.org/jira/browse/DERBY-31 Project: Derby Type: New Feature Components: JDBC Environment: ALL Reporter: Ali Demir 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://nagoya.apache.org/jira/secure/Administrators.jspa - If you want more information on JIRA, or have a bug to report see: http://www.atlassian.com/software/jira
[jira] Commented: (DERBY-31) Statement.setQueryTimeout() support.
[ http://nagoya.apache.org/jira/browse/DERBY-31?page=comments#action_56831 ] Ali Demir commented on DERBY-31: When a time out is set to n millis, and if the query takes n+m millis, then caller would expect that time out occurs around n millis and not wait until n+m. Also, when the timeout occurs, the processing of the query should have been stopped and there should not be any locks held after timeout. Therefore, a simple solution that simply checks how long the query processing is taking is not enough. The internals of the system need to know that it should stop the query and release any locks held by it when the timeout occurs. Statement.setQueryTimeout() support. Key: DERBY-31 URL: http://nagoya.apache.org/jira/browse/DERBY-31 Project: Derby Type: New Feature Components: JDBC Environment: ALL Reporter: Ali Demir 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://nagoya.apache.org/jira/secure/Administrators.jspa - If you want more information on JIRA, or have a bug to report see: http://www.atlassian.com/software/jira
