Better handling of bad connection errors and specifying server protocol. #152
Conversation
… be added in to an error chain to signal that a connection should no longer be used and to retry with a new connection. Updated ThriftServiceClient error handling. When a Thrift request fails we are now checking the error message for the term 'Invalid SessionHandle'. If it is present we are adding a bad connection error to the error chain. Searching in the error message is clumsy and fragile but there doesn't currently appear to be another way to get the information. Updated the WithServerHostname function to handle host names prefixed by 'http:' or 'https:' to allow users to specify which protocol to use. This is in response to github issue databricks#140. Signed-off-by: Raymond Cypher <raymond.cypher@databricks.com>
rcypher-databricks
requested review from
mattdeekay,
susodapop,
yunbodeng-db and
andrefurlan-db
as code owners
| // If the passed error indicates an invalid session we inject a bad connection error | ||
| // into the error stack. This allows the for retrying with a new connection. | ||
| s := err.Error() | ||
| if strings.Contains(s, "Invalid SessionHandle") { |
There was a problem hiding this comment.
we need to be very careful with session state. If the user expects some state - temp views, etc - and we just open a new session in the background, it may be very hard to understand and debug what is going on.
There was a problem hiding this comment.
In the context of database/sql, if the user is using methods on the sql.DB object directly (e.g. db.QueryContext(...)) then they cannot rely on maintaining per-session state anyway because these methods might create new connections or reuse an arbitrary connection from the pool.
If a user does need to rely on per-session state, then doing so correctly requires explicitly grabbing a connection and then calling methods on the connection. In that case, it's the user's responsibility to manage the connection lifecycle, which includes handling errors and accounting for the possibility that a connection may end up in a bad state.
| // into the error stack. This allows the for retrying with a new connection. | ||
| s := err.Error() | ||
| if strings.Contains(s, "Invalid SessionHandle") { | ||
| err = dbsqlerrint.NewBadConnectionError(err) |
There was a problem hiding this comment.
Out of curiosity, looks like the NewBadConnectionError will be wrapped again in a NewRequestError. Would it be better to just return the error as is? Or is the client expecting a NewRequestError?
Added a new error type that identifies as driver.ErrBadConn. This can be added in to an error chain to signal that a connection should no longer be used and to retry with a new connection.
Updated ThriftServiceClient error handling. When a Thrift request fails we are now checking the error message for the term 'Invalid SessionHandle'. If it is present we are adding a bad connection error to the error chain. Searching in the error message is clumsy and fragile but there doesn't currently appear to be another way to get the information.
Updated the WithServerHostname function to handle host names prefixed by 'http:' or 'https:' to allow users to specify which protocol to use. This is in response to github issue #140.