read_timeout option ignored

[traffetseder]

While debugging another timeout issue, I noticed read_timeout isn't used anywhere in the code:
https://github.com/uber/vertica-python/search?q=read_timeout&type=Code
(it's only setting the default value, but the option isn't used anywhere and the other mention is in the README).

[dennisobrien]

For reference, this is how the Ruby version does it:
https://github.com/wvanbergen/vertica/blob/285875db5bc3209fb3bb5312a8c4bd485ff3eb45/lib/vertica/connection.rb#L382

I don't think that translates easily into a single line with Python.

[HaymanLiron]

I also encountered this issue. I'm working on a fix for this. I think in the meantime the read_timeout parameter should be removed. It is confusing for users who set this parameter to something other than 600 and then see that it doesn't change anything.

[sitingren]

@HaymanLiron That's great, thank you for your help!

[pcerny]

I recently also hit this issue. My use case requires two timeouts, one for establishing connection with Vertica and one for waiting to query response. When the Vertica node is healthy, there is no reason to have 2 timeouts but when client tries to connect to node with some issue (TCP socket still open but Vertica node does not respond to it) it is more than useful. And yes, we hit this unlikely scenario several times during HW malfunction.

@sitingren Are there any plans to support two timeouts?
And what about easier timeout identification by proper use of already existing errors.TimedOutError
Additionally, I think that issue #83 could be linked with this one because it discuss another aspect of query timeout, i.e. it is only client side error and query continues running on server side. It would be nice to have interruptable flag like ruby implementation has. I would vote for extending ruby functionality by reestablishing session and calling close_session() just after query timeout in case connection was created with interruptable flag or may by query_cleanup flag.

@HaymanLiron Any progress with implementation of the fix? How exactly do you plan to add support for read_timeout? Do you preserve connection_timeout?

[sitingren]

@pcerny I agree to use existing errors.TimedOutError and I also think it's better to rename 'connection_timeout' option to a more accurate name.

As of today, the 'connection_timeout' option is actually setting socket timeout, which means it sets the maximum wait time for:

1. Establishing a TCP connection to each server address.
2. A single call to socket read/write operation (socket.recv(), socket.sendall()).

Therefore, the 'connection_timeout' option acts on both vertica_python.connect() (i.e. establishing connection with Vertica) and cursor.execute() (i.e. waiting to query response). Currently, 1&2 set to the same value but it is possible to separate them. Will this meet your needs?

We propose to have the following two timeouts:

1. connect_timeout/login_timeout: the maximum time spent waiting for the client connects to the server, which includes the time to try establishing TCP connections to multiple addresses if load balancing is turned on, the time to setup ssl, the time to setup authentication, etc.
2. query_timeout: the maximum time spent waiting for a query to finish. This timeout should send a cancel command to the server to cancel that session's query while the socket is still open.

They may intersect with the socket timeout and increase the complexity. As far as I know, there are timeout decorator tools that can be utilized to wrap functions and achieve the goal of those timeouts somewhat. We'll definitely clear up misunderstandings about existing parameters soon, but adding new timeouts might not be our top-priority task.