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

for downloading object, be pessimistic and check how many bytes got c… #3790

Closed
wants to merge 1 commit into from
Closed

for downloading object, be pessimistic and check how many bytes got c… #3790

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
10 changes: 9 additions & 1 deletion pkg/objstore/objstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,17 @@ func DownloadFile(ctx context.Context, logger log.Logger, bkt BucketReader, src,
}()
defer runutil.CloseWithLogOnErr(logger, f, "download block's output file")

if _, err = io.Copy(f, rc); err != nil {
atts, err := bkt.Attributes(ctx, src)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will double the number of API calls required to download a file. I don't think we should do it.

Copy link
Author

Choose a reason for hiding this comment

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

if don't do it. sometimes return value of io.Copy was less than the legnth of on the object storage.
the file was not complete, but was not err be return.
so ,the thanos compact will change state to [halt]. halt meas ....
if do it, thanos compact will retry.
i don't know some graceful approach to fix this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

if don't do it. sometimes return value of io.Copy was less than the legnth of on the object storage.

Looks an issue with a specific backend storage that shouldn't be fixed here. Which backend storage are you using?

Copy link
Author

Choose a reason for hiding this comment

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

S3

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirm I believe this is not the right place to address it, for a couple of reasons:

  1. We shouldn't introduce additional API calls
  2. If the reader returned by S3 Get() can return partial response without erroring out, then the problem is widespread across the whole codebase (any usage of Get()) so fixing it here would just cover a small amount of use cases

Checking the logic to have a better proposal

Copy link
Contributor

@pracucci pracucci Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

See: #2805 (comment)

Let's continue the discussion on the issue.

if err != nil {
return errors.Wrapf(err, "attributes file %s", src)
}
copied, err := io.Copy(f, rc)
if err != nil {
return errors.Wrap(err, "copy object to file")
}
if copied != atts.Size {
return errors.Errorf("copy object to file %s. object length %v but got %v", src, atts.Size, copied)
}
return nil
}

Expand Down