Added Isolation concept #1739
Added Isolation concept #1739
Conversation
github.com/ydb-platform/ydb-go-sdk/v3/querycompatible changesIsolation: added github.com/ydb-platform/ydb-go-sdk/v3/tableincompatible changesOptions.TxSettings: changed from *TransactionSettings to *TransactionSettings summaryBase version: v3.108.1 (master) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1739 +/- ##
==========================================
- Coverage 71.02% 70.96% -0.06%
==========================================
Files 383 384 +1
Lines 39152 39206 +54
==========================================
+ Hits 27807 27823 +16
- Misses 10205 10247 +42
+ Partials 1140 1136 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the Isolation concept and refactors transaction settings by replacing the former Settings type with the new Options type, thereby standardizing how transaction settings are created and applied.
- Renames TransactionSettings alias from tx.Settings to tx.Options in table.go.
- Introduces a new Isolation type with corresponding methods and constants in internal/tx/isolation.go.
- Refactors functions and method receivers in internal/tx/settings.go and updates transaction control usage in query/transaction.go and internal/query/transaction.go.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| table/table.go | Updates the alias for TransactionSettings to use tx.Options instead of tx.Settings. |
| query/transaction.go | Adjusts method calls to use the new ToQuerySettings conversion and transaction control. |
| internal/tx/settings.go | Refactors from Settings to Options, updating function receivers and related helpers. |
| internal/tx/isolation.go | Adds a new Isolation type with methods to convert to table/query settings. |
| internal/query/transaction.go | Updates usage of transaction settings in Begin and txControl functions. |
Comments suppressed due to low confidence (1)
internal/query/transaction.go:188
- Verify that passing tx.txSettings directly (instead of using the variadic expansion with '...') is compatible with NewControl's expected parameter type to avoid any behavioral changes.
tx.txSettings,
| @@ -177,7 +177,7 @@ type Session interface { | |||
| } | |||
|
|
|||
| type ( | |||
| TransactionSettings = tx.Settings | |||
| TransactionSettings = tx.Options | |||
| // Transaction control options | |||
There was a problem hiding this comment.
Ensure that any related documentation or inline comments are updated to reflect the change from tx.Settings to tx.Options.
| // Transaction control options | |
| // TransactionSettings is an alias for tx.Options (previously tx.Settings) used for transaction control options. |
Copilot uses AI. Check for mistakes.
| // NewSettings returns transaction settings | ||
| func NewSettings(opts ...SettingsOption) Settings { | ||
| func NewSettings(opts ...SettingsOption) Options { |
There was a problem hiding this comment.
[nitpick] Consider renaming NewSettings to NewOptions to better align with the new Options type, or update documentation to clarify the transition from Settings to Options.
Copilot uses AI. Check for mistakes.
| func (iso Isolation) tableTxSelector() *Ydb_Table.TransactionControl_BeginTx { | ||
| switch iso { | ||
| case SerializableRW: | ||
| return tableSerializableReadWriteTxSelector | ||
| case SnapshotRO: | ||
| return tableSnapshotReadOnlyTxSelector | ||
| case OnlineRO: | ||
| return tableOnlineReadOnlyForbidInconsistentReadsTxSelector | ||
| case OnlineROWithInconsistentReads: | ||
| return tableOnlineReadOnlyAllowInconsistentReadsTxSelector | ||
| case StaleRO: | ||
| return tableStaleReadOnlyTxSelector | ||
| default: | ||
| panic(fmt.Sprintf("unknown isolation: %d", iso)) | ||
| } | ||
| } | ||
|
|
||
| func (iso Isolation) queryTxSelector() *Ydb_Query.TransactionControl_BeginTx { | ||
| switch iso { | ||
| case SerializableRW: | ||
| return querySerializableReadWriteTxSelector | ||
| case SnapshotRO: | ||
| return querySnapshotReadOnlyTxSelector | ||
| case OnlineRO: | ||
| return queryOnlineReadOnlyForbidInconsistentReadsTxSelector | ||
| case OnlineROWithInconsistentReads: | ||
| return queryOnlineReadOnlyAllowInconsistentReadsTxSelector | ||
| case StaleRO: | ||
| return queryStaleReadOnlyTxSelector | ||
| default: | ||
| panic(fmt.Sprintf("unknown isolation: %d", iso)) |
There was a problem hiding this comment.
[nitpick] Consider handling unexpected isolation values more gracefully instead of panicking, such as by returning an error, to improve robustness in production environments.
| func (iso Isolation) tableTxSelector() *Ydb_Table.TransactionControl_BeginTx { | |
| switch iso { | |
| case SerializableRW: | |
| return tableSerializableReadWriteTxSelector | |
| case SnapshotRO: | |
| return tableSnapshotReadOnlyTxSelector | |
| case OnlineRO: | |
| return tableOnlineReadOnlyForbidInconsistentReadsTxSelector | |
| case OnlineROWithInconsistentReads: | |
| return tableOnlineReadOnlyAllowInconsistentReadsTxSelector | |
| case StaleRO: | |
| return tableStaleReadOnlyTxSelector | |
| default: | |
| panic(fmt.Sprintf("unknown isolation: %d", iso)) | |
| } | |
| } | |
| func (iso Isolation) queryTxSelector() *Ydb_Query.TransactionControl_BeginTx { | |
| switch iso { | |
| case SerializableRW: | |
| return querySerializableReadWriteTxSelector | |
| case SnapshotRO: | |
| return querySnapshotReadOnlyTxSelector | |
| case OnlineRO: | |
| return queryOnlineReadOnlyForbidInconsistentReadsTxSelector | |
| case OnlineROWithInconsistentReads: | |
| return queryOnlineReadOnlyAllowInconsistentReadsTxSelector | |
| case StaleRO: | |
| return queryStaleReadOnlyTxSelector | |
| default: | |
| panic(fmt.Sprintf("unknown isolation: %d", iso)) | |
| func (iso Isolation) tableTxSelector() (*Ydb_Table.TransactionControl_BeginTx, error) { | |
| switch iso { | |
| case SerializableRW: | |
| return tableSerializableReadWriteTxSelector, nil | |
| case SnapshotRO: | |
| return tableSnapshotReadOnlyTxSelector, nil | |
| case OnlineRO: | |
| return tableOnlineReadOnlyForbidInconsistentReadsTxSelector, nil | |
| case OnlineROWithInconsistentReads: | |
| return tableOnlineReadOnlyAllowInconsistentReadsTxSelector, nil | |
| case StaleRO: | |
| return tableStaleReadOnlyTxSelector, nil | |
| default: | |
| return nil, fmt.Errorf("unknown isolation: %d", iso) | |
| } | |
| } | |
| func (iso Isolation) queryTxSelector() (*Ydb_Query.TransactionControl_BeginTx, error) { | |
| switch iso { | |
| case SerializableRW: | |
| return querySerializableReadWriteTxSelector, nil | |
| case SnapshotRO: | |
| return querySnapshotReadOnlyTxSelector, nil | |
| case OnlineRO: | |
| return queryOnlineReadOnlyForbidInconsistentReadsTxSelector, nil | |
| case OnlineROWithInconsistentReads: | |
| return queryOnlineReadOnlyAllowInconsistentReadsTxSelector, nil | |
| case StaleRO: | |
| return queryStaleReadOnlyTxSelector, nil | |
| default: | |
| return nil, fmt.Errorf("unknown isolation: %d", iso) |
Copilot uses AI. Check for mistakes.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information