Skip to content
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

feat: introducing warehouse repo withTx #4042

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Oct 30, 2023

Description

  • Introducing WithTx to use wherever transactions are used.
  • Also, this is a follow-up on refactor: uploads updates #4045 where I am doing a cleanup for updating fields for the uploads table. WithTx is required during reporting where we update fields and report to reporting in the same transaction.

Linear Ticket

  • Resolves PIPE-462

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (6d2db45) 71.52% compared to head (7d12055) 71.55%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4042      +/-   ##
==========================================
+ Coverage   71.52%   71.55%   +0.02%     
==========================================
  Files         373      373              
  Lines       54838    54827      -11     
==========================================
+ Hits        39224    39231       +7     
+ Misses      13275    13263      -12     
+ Partials     2339     2333       -6     
Files Coverage Δ
enterprise/reporting/error_index/types.go 42.85% <ø> (ø)
warehouse/internal/repo/upload.go 91.01% <ø> (ø)
warehouse/internal/repo/table_upload.go 91.55% <85.71%> (+1.52%) ⬆️
warehouse/internal/repo/load.go 80.32% <79.41%> (+4.57%) ⬆️
warehouse/internal/repo/repo.go 55.55% <42.85%> (-44.45%) ⬇️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


if err := f(tx); err != nil {
if rollbackErr := tx.Rollback(); rollbackErr != nil {
return fmt.Errorf("rollback transaction for %w: %w", err, rollbackErr)
Copy link
Collaborator

@fracasula fracasula Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is like a Nando's double 🐔 wrap but with errors 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit skeptical here. Should we ignore rollback error?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, it's OK. It works perfectly fine and it is possible to unwrap both errors 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware that is possible to wrap two errors that the same time, cool

defer func() { _ = stmt.Close() }()

for _, loadFile := range loadFiles {
metadata := fmt.Sprintf(`{"content_length": %d, "destination_revision_id": %q, "use_rudder_storage": %t}`, loadFile.ContentLength, loadFile.DestinationRevisionID, loadFile.UseRudderStorage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] I would prefer a proper json.Marshal(), IMO is cleaner and safer


if err := f(tx); err != nil {
if rollbackErr := tx.Rollback(); rollbackErr != nil {
return fmt.Errorf("rollback transaction for %w: %w", err, rollbackErr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware that is possible to wrap two errors that the same time, cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants