update offset bugfix #1968
update offset bugfix #1968
Conversation
summaryInferred base version: v3.121.0 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1968 +/- ##
==========================================
- Coverage 73.97% 73.82% -0.16%
==========================================
Files 394 395 +1
Lines 34454 34555 +101
==========================================
+ Hits 25489 25510 +21
- Misses 7842 7910 +68
- Partials 1123 1135 +12
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:
|
- Optimize buildPartitionOffsetsMap: use sessionInfo map for O(1) lookup instead of O(n*m) search - Refactor tests: add helper methods, remove duplication, improve structure - Remove Given-When-Then comments as requested - Fix linter issues: line length, return statements, formatting
🌋 SLO Test ResultsStatus: 🟡 6 workloads tested • 3 workloads with warnings
Generated by ydb-slo-action |
|
|
||
| req := r.createUpdateOffsetRequest(ctx, batch, tx) | ||
| updateOffsetInTransactionErr := retry.Retry(ctx, func(ctx context.Context) (err error) { | ||
| if r.batchTxStorage.Add(tx, batch) { |
|
|
||
| tx.OnBeforeCommit(r.txBeforeCommitFn(tx)) | ||
|
|
||
| tx.OnCompleted(func(err error) { |
There was a problem hiding this comment.
Получится тоже вынести в отдельный метод?
| po := &partitionOffsets[i] | ||
| info, ok := sessionInfoMap[po.PartitionSessionID] | ||
| if !ok { | ||
| // Skip if session info not found (should not happen in normal flow) |
There was a problem hiding this comment.
Вот тут тогда надо кидать ошибку, чтобы не скрывать внутренние ошибки если они появятся.
пропущенный коммит отлаживать будет заметно сложнее, чем явную ошибку.
| ) | ||
|
|
||
| type BatchTxStorageTestSuite struct { | ||
| suite.Suite |
There was a problem hiding this comment.
Нужны ли тут suite? этот способ запуска тестов новый для go sdk
… testify suite with require, streamline test functions, and enhance helper methods for better readability and maintainability.
| // GetOrCreateTransactionBatches gets or creates a transaction batches handler for the given transaction. | ||
| // It returns the handler and a flag indicating whether the transaction is new (true) or already existed (false). | ||
| // This method is thread-safe. | ||
| func (s *batchTxStorage) GetOrCreateTransactionBatches(transaction tx.Transaction) (*transactionBatches, bool) { |
There was a problem hiding this comment.
Тут я бы использовал именованные результаты - чтобы было понятно что означает bool без чтения длинного комментария, как в
https://pkg.go.dev/sync@go1.25.5#Map.LoadAndDelete
| s.m.Unlock() | ||
|
|
||
| if !ok { | ||
| return nil, errNoBatches |
There was a problem hiding this comment.
тут заворачивал бы ошибки в WithTrace - чтобы видеть где конкретно она возникла, когда одна и та же ошибка может кидаться в разных местах кода.
| po := &partitionOffsets[i] | ||
| info, ok := sessionInfoMap[po.PartitionSessionID] | ||
| if !ok { | ||
| return nil, fmt.Errorf("session info not found for partition session ID %d", po.PartitionSessionID) |
There was a problem hiding this comment.
и тут тоже withtrace, да и вообще - везде
…s with stack traces and improve return values for transaction batch creation.
…or improved readability by formatting function parameters and error messages across multiple lines.
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