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.

Sign up for GitHub

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

fix: backup progress should not add block failed to upload to successful count #209

Merged
merged 1 commit into from
Nov 12, 2024
Merged

fix: backup progress should not add block failed to upload to successful count #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion backupbackingimage/backupbackingimage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@
return err
}

return bsDriver.Write(blkFile, rs)
err = bsDriver.Write(blkFile, rs)
return err

Check warning on line 268 in backupbackingimage/backupbackingimage.go

View check run for this annotation

Codecov / codecov/patch

backupbackingimage/backupbackingimage.go#L267-L268 

Added lines #L267 - L268 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider using a returned err variable instead, so updates to the variable won't be missed.

}

// isBlockBeingProcessed check if the block is being processed by other goroutine and prevent redundant work
Expand Down
3 changes: 2 additions & 1 deletion deltablock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,8 @@ func backupBlock(bsDriver BackupStoreDriver, config *DeltaBackupConfig,
return errors.Wrapf(err, "failed to get transfer data size during saving blocks")
}

if err := bsDriver.Write(blkFile, rs); err != nil {
err = bsDriver.Write(blkFile, rs)
if err != nil {
Comment on lines +428 to +429
Copy link

@coderabbitai coderabbitai bot Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Fix Approved for deltablock.go:428.

However, similar error handling issues were found in backupbackingimage.go:267 where the error from bsDriver.Write is not assigned. Please ensure all bsDriver.Write calls properly handle errors to avoid counting failed uploads as successful.

  • backupbackingimage/backupbackingimage.go:267
🔗 Analysis chain

LGTM! Fix properly captures Write errors.

The change correctly captures and propagates errors from the Write operation, ensuring failed block uploads are not counted as successful.

Let's verify if there are similar patterns in the codebase that might need the same fix:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for similar patterns where Write errors might not be captured
# Test: Search for direct Write calls without error assignment
ast-grep --pattern 'bsDriver.Write($_, $_)
if err != nil'

Length of output: 59

Script:

#!/bin/bash
# Description: Look for bsDriver.Write calls without error assignment
# Test: Search for bsDriver.Write calls that do not assign the returned error to a variable
ast-grep --pattern 'bsDriver.Write($_, $_)' --filter '!(parent(expr_stmt) and parent(expr_stmt).expr = "err = bsDriver.Write($_, $_)")'

Length of output: 325

Script:

#!/bin/bash
# Description: Look for bsDriver.Write calls without error assignment
# Test: Search for bsDriver.Write calls not assigned to the err variable
rg -n 'bsDriver\.Write\s*\([^)]*\)\s*(?!\s*err\s*=)'

Length of output: 332

Script:

#!/bin/bash
# Description: Look for bsDriver.Write calls without error assignment
# Test: Search for bsDriver.Write calls not assigned to the err variable using PCRE2
rg -n --pcre2 'bsDriver\.Write\s*\([^)]*\)\s*(?!\s*err\s*=)'

Length of output: 196

Copy link
Member Author

Choose a reason for hiding this comment

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

So clever!

Copy link

Choose a reason for hiding this comment

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

@derekbit, happy to help!

return errors.Wrapf(err, "failed to write data during saving blocks")
}

Expand Down