Skip to content

Stream optional interfaces #11

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

Merged
merged 39 commits into from
Oct 17, 2024
Merged

Conversation

ahouene
Copy link
Contributor

@ahouene ahouene commented Oct 25, 2022

Per #3, and inspired by fs.FS optional interfaces (e.g. fs.GlobFS), this PR introduces two optional interfaces: WriterStorage and ReaderStorage.

// A ReaderStorage is an Interface providing an optimized way to create an io.ReadCloser.
type ReaderStorage interface {
	Interface
	// NewReader returns an io.ReadCloser, allowing stream reading
	// of named value from the underlying backend.
	NewReader(context.Context, string) (io.ReadCloser, error)
}

// A WriterStorage is an Interface providing an optimized way to create an io.WriteCloser.
type WriterStorage interface {
	Interface
	// NewWriter returns an io.WriteCloser, allowing stream writing
	// to named key in the underlying backend.
	NewWriter(context.Context, string) (io.WriteCloser, error)
}

Those interfaces are created using global functions:

func NewReader(ctx context.Context, st Interface, name string) (io.ReadCloser, error)

func NewWriter(ctx context.Context, st Interface, name string) (io.WriteCloser, error)

In case st doesn't satisfy ReaderStorage or WriterStorage, it will be wrapped in a struct, allowing abstraction but no performance gain.

Corresponding tests have been implemented.

Closes #3

ahouene added 6 commits July 26, 2022 11:31
Created simpleblob.NewReader and simpleblob.NewWriter functions, along with ReaderStorage and WriterStorage interface types.
Those functions allow to provide an optimized way to write to and read from backend if those interfaces are satsfied.
If an interface is not satisfied by backend, a fallback implementation is used, akin to `fs.GlobFS`.
Provide io.ReadCloser and io.WriteCloser to interact with files to way the `os` package does it.
The `os` package has byte-slice based functions that rely on streams under the hood, we're doing the same here.
The Object returned by MinIO client satisfies io.ReadCloser, so this implementation is very straightforward.

For writing, the MinIO client takes a reader and needs the full size of the stored object.
Thus, we'd need to exhaust the writer into a buffer, take its size and provide it to the client,
which is basically what the fallback implementation of NewWriter does.
Confront the results from simpleblob.NewReader and simpleblob.NewWriter with simpleblob.Interface.
@ahouene ahouene requested a review from wojas October 25, 2022 15:30
Copy link
Member

@wojas wojas left a comment

Choose a reason for hiding this comment

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

Mostly nits and improvements, the logic LGTM, except for the fs writer.

Copy link
Member

@wojas wojas left a comment

Choose a reason for hiding this comment

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

First pass, will do a more in depth one soon.

@ahouene ahouene requested a review from wojas October 10, 2023 08:01
@@ -0,0 +1,90 @@
package fs

Choose a reason for hiding this comment

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

This looks solid, I guess you have looked at other similar solutions like this one:
https://github.com/google/renameio

just to see we are not missing any corner cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the recommendation. I gave it a look and while it looks like it can handle a broader set of situations (that we don't need here), I didn't see something that would benefit us here.

@ahouene ahouene linked an issue Oct 16, 2024 that may be closed by this pull request
Copy link

@johanlantz johanlantz left a comment

Choose a reason for hiding this comment

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

Nothing to add since last time. If @wojas comments are all closed it seems like it's time to finalize this.

@ahouene ahouene merged commit 223250b into PowerDNS:main Oct 17, 2024
2 checks passed
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.

Avoid byte slice parameters Implement streaming writing
3 participants