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

Use blob.OutputBuffer in blob.Reader interface instead of internal gather.WriteBuffer #1452

Merged
merged 2 commits into from Nov 3, 2021

Conversation

dzaninovic
Copy link
Contributor

@dzaninovic dzaninovic commented Oct 29, 2021

Exported blob.Storage interface can't be implemented as GetBlob() is using internal gather.WriteBuffer as an output argument type.

My use case is to intercept all Kopia calls to blob.Storage so I can recreate new underlying blob.Storage on the fly if AWS session expires in the middle of a long operation like creating the snapshot.

I am also using this blob.Storage wrapper to log and keep statistics of all calls to blob.Storage.

Proposed changes change the GetBlob() argument type to blob.OutputBuffer.

make command ran all tests successfully

Forum discussion reference:
https://kopia.discourse.group/t/implementing-repo-blob-reader-interface/706/4

@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #1452 (4f53236) into master (a162a61) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1452      +/-   ##
==========================================
- Coverage   68.48%   68.44%   -0.04%     
==========================================
  Files         367      367              
  Lines       28920    28920              
==========================================
- Hits        19805    19795      -10     
- Misses       7476     7482       +6     
- Partials     1639     1643       +4     
Impacted Files Coverage Δ
repo/blob/storage.go 100.00% <ø> (ø)
repo/blob/filesystem/filesystem_storage.go 66.31% <100.00%> (ø)
repo/blob/logging/logging_storage.go 100.00% <100.00%> (ø)
repo/blob/readonly/readonly_storage.go 62.96% <100.00%> (ø)
repo/blob/retrying/retrying_storage.go 100.00% <100.00%> (ø)
repo/blob/sftp/sftp_storage.go 56.90% <100.00%> (ø)
repo/blob/sharded/sharded.go 86.70% <100.00%> (ø)
repo/content/content_cache_metadata.go 71.91% <0.00%> (-7.87%) ⬇️
repo/content/content_manager_lock_free.go 71.64% <0.00%> (-2.24%) ⬇️
internal/cache/persistent_lru_cache.go 82.50% <0.00%> (-1.88%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a162a61...4f53236. Read the comment docs.

@jkowalski
Copy link
Contributor

I think it would be simpler to add interface like this:

// OutputBuffer is implemented by *gather.WriteBuffer
type OutputBuffer interface {
	io.Writer

	Reset()
	Length() int
}

and change *gather.WriteBuffer to storage.OutputBuffer everywhere.

Also, if we renamed Length to Len, I believe bytes.Buffer would match this interface as well, which might be convenient if gather.WriteBuffer is not needed.

@dzaninovic
Copy link
Contributor Author

Good idea, that would also give me access to Length() from the wrapper.
I will work on implementing that.

@dzaninovic dzaninovic changed the title Use io.Writer in blob.Storage interface instead of internal gather.WriteBuffer Use blob.OutputBuffer in blob.Reader interface instead of internal gather.WriteBuffer Nov 2, 2021
@dzaninovic
Copy link
Contributor Author

I made the change you suggested but had to add ToByteSlice() and Append() to the interface as they were needed by eventuallyConsistentStorage and mapStorage implementations.

I looked into bytes.Buffer but that is not an interface so change would be more complicated.

@jkowalski
Copy link
Contributor

I don't think we need Append and ToByteSlice - I tried adding a commit to this PR but don't have permissions to your branch, Can you add this:

jkowalski@36d30b3

@dzaninovic
Copy link
Contributor Author

dzaninovic commented Nov 3, 2021

I cherry-picked your commit and rebased this branch to the latest master.
All tests make ran are passing.

@jkowalski
Copy link
Contributor

Thank you for the contribution!

BTW. do you want to perhaps sync up on Slack, since you're consuming Kopia in some novel ways, I'd like to make sure we stay in touch as there may be low-level changes that might break you going forward (as the golang API is not really stable yet).

@jkowalski jkowalski merged commit 540910e into kopia:master Nov 3, 2021
@dzaninovic
Copy link
Contributor Author

Thank you for merging the pull request!
I joined Slack and will monitor the channels for updates.

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

Successfully merging this pull request may close these issues.

None yet

2 participants