feat(dbapi): wire timeout parameter through Connection to execute_sql#16497
feat(dbapi): wire timeout parameter through Connection to execute_sql#16497waiho-gumloop wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Add a timeout property to Connection and pass timeout= to execute_sql() in the three DBAPI code paths that currently omit it: snapshot reads, transaction statements, and autocommit DML. When timeout is None (the default), timeout= is not passed, preserving the existing behavior of using gapic_v1.method.DEFAULT (3600s). Fixes googleapis#16492
There was a problem hiding this comment.
Code Review
This pull request introduces a timeout property to the Spanner DB-API Connection class, allowing users to specify gRPC deadlines for SQL operations. The timeout is propagated to execute_sql calls within both Connection and Cursor methods, and comprehensive unit tests have been added to verify this functionality. Feedback was provided to use the public timeout property instead of the private _timeout attribute for better encapsulation and consistency across the codebase.
| if self._timeout is not None: | ||
| kwargs["timeout"] = self._timeout |
There was a problem hiding this comment.
Consider using the public timeout property instead of the private _timeout attribute for consistency, even within the class methods.
| if self._timeout is not None: | |
| kwargs["timeout"] = self._timeout | |
| if self.timeout is not None: | |
| kwargs["timeout"] = self.timeout |
| if self.connection._timeout is not None: | ||
| kwargs["timeout"] = self.connection._timeout |
There was a problem hiding this comment.
It is better to use the public timeout property of the connection instead of accessing the private _timeout attribute directly. This ensures better encapsulation and consistency with how other connection properties (like autocommit or read_only) are accessed throughout the codebase.
| if self.connection._timeout is not None: | |
| kwargs["timeout"] = self.connection._timeout | |
| if self.connection.timeout is not None: | |
| kwargs["timeout"] = self.connection.timeout |
| if self.connection._timeout is not None: | ||
| kwargs["timeout"] = self.connection._timeout |
There was a problem hiding this comment.
It is better to use the public timeout property of the connection instead of accessing the private _timeout attribute directly. This ensures better encapsulation and consistency with how other connection properties are accessed.
| if self.connection._timeout is not None: | |
| kwargs["timeout"] = self.connection._timeout | |
| if self.connection.timeout is not None: | |
| kwargs["timeout"] = self.connection.timeout |
Summary
Add a
timeoutproperty toConnectionand passtimeout=toexecute_sql()in the three DBAPI code paths that currently omit it: snapshot reads, transaction statements, and autocommit DML.Fixes #16492
Background
_SnapshotBase.execute_sql()accepts atimeoutparameter that controls the gRPC deadline for theExecuteStreamingSqlRPC. When not provided, it defaults togapic_v1.method.DEFAULT, which resolves todefault_timeout=3600.0in the transport layer.The DBAPI calls
execute_sql()in three locations, none of which passtimeout=:cursor._handle_DQL_with_snapshot()callssnapshot.execute_sql(sql, params, param_types, request_options=...)withouttimeout=connection.run_statement()callstransaction.execute_sql(sql, params, param_types=..., request_options=...)withouttimeout=cursor._do_execute_update_in_autocommit()callstransaction.execute_sql(sql, params=..., param_types=..., last_statement=True)withouttimeout=This means DBAPI consumers (SQLAlchemy, Django, raw DBAPI) cannot control the gRPC deadline for individual statements.
Changes
connection.pyself._timeout = Noneto__init__timeoutproperty and settertimeout=self._timeoutinrun_statement()when setcursor.pytimeout=self.connection._timeoutin_handle_DQL_with_snapshot()when settimeout=self.connection._timeoutin_do_execute_update_in_autocommit()when setWhen
timeoutisNone(the default),timeout=is not passed, preserving the existing behavior of usinggapic_v1.method.DEFAULT.Usage
Tests
Added 7 unit tests covering default value, property getter/setter, and timeout propagation in all three code paths.
Related