feat: allow setting a timeout for mysql try lock#1200
feat: allow setting a timeout for mysql try lock#1200vgarvardt wants to merge 2 commits intogolang-migrate:masterfrom
Conversation
|
Tests are failing with that has nothing to do with the PR |
vgarvardt
force-pushed
the
feat/try-lock-timeout
branch
from
b58a1ac to
6834b7b
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes the MySQL driver’s advisory lock timeout configurable instead of using a hardcoded 10-second value.
Changes:
- Introduces
DefaultTryLockTimeoutSecand a newTryLockTimeoutSecfield onConfig. - Wires a new
x-try-lock-timeoutURL parameter throughOpeninto the driver configuration and uses it inLockwhen callingGET_LOCK. - Documents the new
x-try-lock-timeoutparameter indatabase/mysql/README.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| database/mysql/mysql.go | Adds configurable try-lock timeout (TryLockTimeoutSec), parses x-try-lock-timeout from the DSN, and uses it in Lock for GET_LOCK. |
| database/mysql/README.md | Documents the new x-try-lock-timeout DSN parameter and its semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tryLockTimeout := DefaultTryLockTimeoutSec | ||
| if m.config.TryLockTimeoutSec != 0 { | ||
| tryLockTimeout = m.config.TryLockTimeoutSec | ||
| } | ||
|
|
There was a problem hiding this comment.
In Lock, treating TryLockTimeoutSec == 0 as a signal to fall back to DefaultTryLockTimeoutSec means that a configured timeout of 0 seconds (e.g. via x-try-lock-timeout=0) is effectively ignored and still results in a 10 second wait, which diverges from MySQL GET_LOCK semantics where 0 is a valid "no wait" value. To make x-try-lock-timeout fully configurable and consistent with the README and MySQL behavior, this logic should distinguish between "unset" and an explicit 0 value (for example by always trusting m.config.TryLockTimeoutSec when set via Open, or by adding an explicit "use default" flag in the config).
| tryLockTimeout := DefaultTryLockTimeoutSec | |
| if m.config.TryLockTimeoutSec != 0 { | |
| tryLockTimeout = m.config.TryLockTimeoutSec | |
| } | |
| tryLockTimeout := m.config.TryLockTimeoutSec |
| tryLockTimeoutParam := customParams["x-try-lock-timeout"] | ||
| tryLockTimeout := DefaultTryLockTimeoutSec | ||
| if tryLockTimeoutParam != "" { | ||
| tryLockTimeout, err = strconv.Atoi(tryLockTimeoutParam) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not parse x-try-lock-timeout as int: %w", err) | ||
| } | ||
| } | ||
|
|
||
| db, err := sql.Open("mysql", config.FormatDSN()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| mx, err := WithInstance(db, &Config{ | ||
| DatabaseName: config.DBName, | ||
| MigrationsTable: customParams["x-migrations-table"], | ||
| NoLock: noLock, | ||
| StatementTimeout: time.Duration(statementTimeout) * time.Millisecond, | ||
| DatabaseName: config.DBName, | ||
| MigrationsTable: customParams["x-migrations-table"], | ||
| NoLock: noLock, | ||
| StatementTimeout: time.Duration(statementTimeout) * time.Millisecond, | ||
| TryLockTimeoutSec: tryLockTimeout, | ||
| }) |
There was a problem hiding this comment.
There are no tests covering the new x-try-lock-timeout behavior (parsing, defaulting, and how it affects GET_LOCK), even though this file already has integration and parameter-validation tests (e.g. for x-no-lock). Please add tests that at least verify successful parsing of valid values (including negative and zero), handling of invalid values, and that the configured timeout is actually used when acquiring the advisory lock.
2 participants
Timeout for
GET_LOCK()for mysql driver is currently hardcoded to 10 seconds. This PR allows setting a custom value for the timeout.