-
Notifications
You must be signed in to change notification settings - Fork 107
update offset bugfix #1968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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.92% 73.91% -0.02%
==========================================
Files 394 395 +1
Lines 34454 34498 +44
==========================================
+ Hits 25471 25500 +29
- Misses 7855 7866 +11
- Partials 1128 1132 +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:
|
- 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
непонятное для чтения
|
|
||
| tx.OnBeforeCommit(r.txBeforeCommitFn(tx)) | ||
|
|
||
| tx.OnCompleted(func(err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Получится тоже вынести в отдельный метод?
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вот тут тогда надо кидать ошибку, чтобы не скрывать внутренние ошибки если они появятся.
пропущенный коммит отлаживать будет заметно сложнее, чем явную ошибку.
| ) | ||
|
|
||
| type BatchTxStorageTestSuite struct { | ||
| suite.Suite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужны ли тут suite? этот способ запуска тестов новый для go sdk
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