Make SQLServer deadlock query retries reusable#14947
Conversation
hashhar
added
the
no-release-notes
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Outdated
Show resolved
Hide resolved
hashhar
force-pushed
the
hashhar/sqlserver-cleanup
branch
from
31a8ad4 to
ca7f5d5
Compare
ssheikin
left a comment
ssheikin
left a comment
There was a problem hiding this comment.
Thanks!
Please squash commits.
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Outdated
Show resolved
Hide resolved
hashhar
force-pushed
the
hashhar/sqlserver-cleanup
branch
from
ca7f5d5 to
5de6556
Compare
| { | ||
| // DDL operations can take out locks against system tables causing queries against them to deadlock | ||
| int maxAttemptCount = 3; | ||
| RetryPolicy<T> retryPolicy = new RetryPolicy<T>() |
There was a problem hiding this comment.
nit: retry policies can be extracted as constants
There was a problem hiding this comment.
that was the initial version but it looks cleaner this way - especially since the policy has no use outside of this very specific case (at-least today).
| return retryOnDeadlock(() -> getTableDataCompression(handle, table), "error when getting table compression info for '%s'".formatted(table)); | ||
| } | ||
|
|
||
| public static <T> T retryOnDeadlock(CheckedSupplier<T> supplier, String attemptLogMessage) |
There was a problem hiding this comment.
Do we need to be so specific to stress the OnDeadlock part? I think there are certain operation that are retry-able where deadlock could be one of them. We may want retry in other places too so maybe having some utility for retries for sql-server could be beneficial.
Just a nit.
There was a problem hiding this comment.
Yes, would be useful. I don't any such cases today. Once we have them we can refactor - refactor without existing usage makes it difficult to decide optimal API.
There was a problem hiding this comment.
Note that when we introduce such retry I expect it to actually live in BaseJdbcClient (for metadata) and JdbcRecordSetProvider (for data).
4 participants
The 2nd commit isn't very useful IMO. Just the first one should be fine.