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

Jump to bottom

b2: Include custom upload headers in large file info - fixes #7744 #7750

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

b2: Include custom upload headers in large file info - fixes #7744 #7750

wants to merge 1 commit into from

Conversation

metadaddy
Copy link
Contributor

What is the purpose of this change?

This update fixes custom upload headers for B2 large files.

Was the change discussed in an issue or in the forum before?

See #7744.

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

ncw
ncw reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

I think this is looking very good :-)

I put a couple of comments inline.

This is verging into full metadata support for b2. Is that your plan for what next? Full metadata support would be very useful.

backend/b2/upload.go Show resolved Hide resolved
fstest/fstests/fstests.go Outdated
Comment on lines 201 to 202
func PutTestContentsOptions(ctx context.Context, t *testing.T, f fs.Fs, file *fstest.Item, contents string, check bool, mimeType string, options ...fs.OpenOption) fs.Object {
var (
Copy link
Member

Choose a reason for hiding this comment

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

I think you can probably add options ...fs.OpenOption as the last parameter of PutTestContentsMetadata for a neater fix.

This needs refactoring to take an options object, but that can be another change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do - in fact, I think I intended to go back and do exactly that!

@metadaddy
Copy link
Contributor Author

This is verging into full metadata support for b2. Is that your plan for what next? Full metadata support would be very useful.

I think that would be relatively straightforward, using the s3 backend as a model. I'll put it on my todo list 🙂

@metadaddy metadaddy force-pushed the b2-custom-headers branch 2 times, most recently from 33a0dc7 to 8e83481 Compare April 13, 2024 01:57
ncw
ncw reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

We are getting there :-)

Please see comments inline.

Thank you

backend/b2/upload.go Show resolved Hide resolved
fstest/fstests/fstests.go Outdated
return err
})
file.Hashes = uploadHash.Sums()
if check {
// Default mtime header for everything except b2
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree with this part...

In rclone's metadata scheme the mtime should always be the name of the key in the metadata and it is the job of the backend to translate it.

So I don't think this should have mtimeHeaderName passed into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that b2 is the sole outlier in using src_last_modified_millis rather than mtime, behavior that is part of the documented interface for b2: https://rclone.org/b2/#modification-times.

The logic here seemed like a better option for now than reworking b2 to use mtime. Reworking b2 to use mtime as well as/instead of src_last_modified_millis seemed out of scope for a change focused on fixing upload headers for large files, and your earlier advice was to merge the PutTestContentsOptions function I broke out for b2 back into PutTestContentsMetadata, so this is where I landed. Happy to move forward in whichever way you suggest!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ncw - I should maybe have mentioned you with that implicit question, and made it more explicit: is it better, in the context of this PR, to:

  • Change b2 so that it supports mtime as well as src_last_modified_millis.
  • Have b2 indicate to PutTestContentsMetadata that it uses src_last_modified_millis rather than mtime (where the code is now).
  • Have b2 use a different function than PutTestContentsMetadata (where the code was).
  • Some other solution I haven't thought of 😄

Copy link
Member

Choose a reason for hiding this comment

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

He he - thank you for the list of options :-)

We don't want src_last_modified_millis to ever escape into the rclone metadata output. Your changes to PutTestContentsMetadata are a bit untidy (mixing different layers of the code) but don't cause this escape since you haven't implemented metadata yet.

For a full metadata implementation we'll need it to appear as mtime to the outside world but that must be saved as a parameter called src_last_modified_millis as that is part of the b2 protocol.

  1. Change b2 so that it supports mtime as well as src_last_modified_millis.

This won't be wasted work as it will be needed for metadata support.

  1. Have b2 indicate to PutTestContentsMetadata that it uses src_last_modified_millis rather than mtime (where the code is now).

I don't like the b2 specific code in a general routine. I've tried to keep backend specific stuff out of fstest/fstests/fstests.go and only use the Features() flags.

  1. Have b2 use a different function than PutTestContentsMetadata (where the code was).

Will need to be undone later when metadata is supported!

  • Some other solution I haven't thought of 😄

:-)

My feeling is probably 1. if it isn't too hard, but 3. would be fine also.

It depends on what your appetite for doing the full metadata is for the b2 backend?

fstest/fstests/fstests.go Outdated Show resolved Hide resolved
@metadaddy
Copy link
Contributor Author

Hi @ncw - I took option 1, adding just enough metadata support to b2 for PutTestContentsMetadata to work. I'll put full metadata support on my todo list. All looks good to me...

% cd backend/b2                                            
% go test -v -remote TestB2:                     
=== RUN   TestUrlEncode
--- PASS: TestUrlEncode (0.00s)
...
PASS
ok  	github.com/rclone/rclone/backend/b2	289.731s

@metadaddy
Copy link
Contributor Author

@ncw Gentle nudge 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncw ncw ncw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@metadaddy @ncw