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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chore] use kv.KVStore also for S3 storage #1113

Merged

Conversation

NyaaaWhatsUpDoc
Copy link
Member

This should solve #1105

It replaces our current S3 storage driver with kv.KVStore using storage.S3 under the hood.

In time we can move to remove the now fairly-redundant storage.Driver interface.

Shoutout to @theSuess who was also working on this at the same time 馃榾

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
@NyaaaWhatsUpDoc
Copy link
Member Author

Thank you @theSuess for the go-store fix!

Copy link
Contributor

@theSuess theSuess left a comment

Choose a reason for hiding this comment

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

Got it working on my machine! One issue in this change needs to be addressed and I've sent you a second PR for go-store which fixes another issue

internal/storage/storage.go Outdated Show resolved Hide resolved
testrig/storage.go Outdated Show resolved Hide resolved
@theSuess
Copy link
Contributor

I've rolled this out on my test instance and while it still allocates more memory than the uploaded file, it's less than previously.

image

@tsmethurst
Copy link
Contributor

Good job nerds, will review now. Is this ready to merge if it lgtm?

@NyaaaWhatsUpDoc
Copy link
Member Author

I've rolled this out on my test instance and while it still allocates more memory than the uploaded file, it's less than previously.

image

You see from the go-store code it only allocates that (5MiB as we have it set) write buffer once, and reuses it for each chunk. I wonder if this means minio is still heavy on memory allocation even when chunking 馃

@NyaaaWhatsUpDoc
Copy link
Member Author

Good job nerds, will review now. Is this ready to merge if it lgtm?

yep go for it!

@tsmethurst tsmethurst merged commit a898160 into superseriousbusiness:main Nov 22, 2022
@matthewp
Copy link
Contributor

Should #1105 have been closed because of this?

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

4 participants