fix: add sql transaction by log when importing #925
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant DB
Client->>Controller: Import(logs)
loop For each log
Controller->>DB: BeginTX()
Controller->>Controller: importLog(ctx, txStore, log)
alt Success
Controller->>DB: Commit()
else Error (serialization/concurrency)
Controller->>DB: Rollback()
Controller->>Client: Return import error
end
end
Controller->>Client: Return result
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/controller/ledger/controller_default.go (2)
237-237: Consider removing unnecessary variable.The
errvariable on line 237 isn't needed since it's not set after the for loop and would always benilat this point.- return err + return nil
217-222: Improve error message formatting.There's a minor issue with the error message string concatenation where "concurrent transaction occurred" is split across two lines in an unusual way.
- return NewErrImport(errors.New("concurrent transaction occur" + - "red, cannot import the ledger")) + return NewErrImport(errors.New("concurrent transaction occurred, cannot import the ledger"))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
deployments/pulumi/go.modis excluded by!**/*.moddeployments/pulumi/go.sumis excluded by!**/*.sum,!**/*.sumgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtest/rolling-upgrades/go.modis excluded by!**/*.modtest/rolling-upgrades/go.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
internal/controller/ledger/controller_default.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (3)
internal/controller/ledger/controller_default.go (3)
207-234: Good transaction management implementation.The code now wraps each log import within its own database transaction, providing stronger atomicity guarantees. This ensures that each log is either fully imported or not imported at all, preventing partial imports that could lead to data inconsistency.
The implementation includes:
- Proper transaction lifecycle management (begin, commit, rollback)
- Comprehensive error handling, especially for concurrent transaction scenarios
- Clean use of a deferred rollback to prevent leaked transactions
240-240: Good method signature update to support transactions.Updating the method signature to accept a
storeparameter allows for the proper use of transactional stores, making the function more flexible and enabling atomic operations.
249-249: Consistent use of the transactional store.All database operations within the
importLogmethod now consistently use the provided transactional store parameter instead of the controller's store. This ensures that all operations are part of the same transaction and will be committed or rolled back together.Also applies to: 255-255, 263-263, 270-270, 275-275, 285-285, 290-290, 298-298
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
==========================================
+ Coverage 81.96% 82.23% +0.27%
==========================================
Files 141 141
Lines 7764 7781 +17
==========================================
+ Hits 6364 6399 +35
+ Misses 1074 1057 -17
+ Partials 326 325 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| switch { | ||
| case errors.Is(err, postgres.ErrSerialization) || | ||
| errors.Is(err, ErrConcurrentTransaction{}): | ||
| return NewErrImport(errors.New("concurrent transaction occur" + |
There was a problem hiding this comment.
Just a quick one -- shouldn't they all be "NewErrImport" ?
There was a problem hiding this comment.
I'm used to define specific errors for business errors.
In the context of the ledger, errors are processed on the api by checking for each kind of error (a bit like a try/catch).
All unknown errors return a 500 and trigger an error log (and trace). Others are translated to a specific business error code.
If we wrap all errors in NewErrImport, technical errors could leak in the api.
I guess we could have two levels of checks api side.
No description provided.