jaydebeapi icon indicating copy to clipboard operation
jaydebeapi copied to clipboard

add query timeout to execute and executemany methods

Open fernandezpablo85 opened this issue 6 years ago • 12 comments

From the PreparedStatement jdbc docs:

By default there is no limit on the amount of time allowed for a running statement to complete.

This changes allow the user to specify a query_timeout in seconds.

fernandezpablo85 avatar Nov 07 '18 15:11 fernandezpablo85

Coverage Status

Coverage increased (+0.2%) to 74.885% when pulling 064364e1a0a014a58da32f0522c13aa337286072 on fernandezpablo85:master into e99a05d5a84e9aa37ff0bac00bd5591336f54402 on baztian:master.

coveralls avatar Nov 07 '18 15:11 coveralls

Coverage Status

Coverage increased (+2.8%) to 77.471% when pulling 2b4a7e561f64cc9898194e92a6997818af26f8b0 on fernandezpablo85:master into e99a05d5a84e9aa37ff0bac00bd5591336f54402 on baztian:master.

coveralls avatar Nov 07 '18 15:11 coveralls

@baztian ?

fernandezpablo85 avatar Nov 18 '18 14:11 fernandezpablo85

@baztian?

fernandezpablo85 avatar Dec 10 '18 16:12 fernandezpablo85

@baztian done. Let me know what you think.

fernandezpablo85 avatar Jan 02 '19 22:01 fernandezpablo85

Thanks again. I don't understand why timeout is extracted from the driver url. Shouldn't the driver already take care of the timeouts if you specify such an url. It is definitely passed to the driver? And would be wondering if this is a JDBC standard way to specify timeouts. What keeps a vendor from not naming the timeout parameter something else?

baztian avatar Jan 03 '19 12:01 baztian

Sorry. You're right. Will send an updated patch later today.

fernandezpablo85 avatar Jan 03 '19 12:01 fernandezpablo85

@baztian I think that is what you originally meant, right?

fernandezpablo85 avatar Jan 03 '19 14:01 fernandezpablo85

One more thought: I know I asked you to change it to be a connection parameter to stay as close to DB-API spec as possible. But I also remember the discussion in #29 where the same options exist. It might be the best way to have the timeout available in both: connection and on a per query basis. This way you have the choice to be DB-API conformant and to have a timeout on a per query basis (but without being DB-API conformant). For me it would be ok to only have the connection version for now. If one needs this later on we can still add it. But feel free to add both variants.

baztian avatar Jan 03 '19 15:01 baztian

@baztian I'm having a tough time setting up the test environment. I created a test that passes a timeout and sees that at least the statement succeeds. Hopefully the CI will run it.

I was thinking of a more thorough test modifying the .java connection Mock to Thread.sleep conditionally if the passed query contains the string "timeout". It was a bit cumbersome but if you want to I can add it.

fernandezpablo85 avatar Jan 03 '19 17:01 fernandezpablo85

Till this PR gets released, we can do this

def _set_stmt_parms(self, prep_stmt, parameters):
        for i in range(len(parameters)):
            prep_stmt.setObject(i + 1, parameters[i])
        prep_stmt.setQueryTimeout(QUERY_TIMEOUT)

jaydebeapi.Cursor._set_stmt_parms = _set_stmt_parms

or use new

AdeshAtole avatar Apr 01 '19 08:04 AdeshAtole

+1

mgarriga avatar Jan 06 '22 20:01 mgarriga