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

KeyStreamerAt #7

Merged
merged 6 commits into from
Sep 16, 2021
Merged

KeyStreamerAt #7

merged 6 commits into from
Sep 16, 2021

Conversation

thomascoquet
Copy link
Contributor

The PR concerns the following changes:

  • introduction of the KeyStreamerAt interface to stream blocks instead of fetching them using KeyReaderAt. This has the benefits of reducing lock contention and allocation of large byte buffers.
  • refacto of existing handlers so that they implement both KeyStreamerAt and the original API KeyReaderAt using a wrapper.

@coveralls
Copy link

coveralls commented Sep 15, 2021

Pull Request Test Coverage Report for Build 1241102608

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 29 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.9%) to 88.624%

Files with Coverage Reduction New Missed Lines %
s3.go 1 72.88%
gcs.go 3 65.91%
http.go 7 52.24%
adapter.go 18 95.96%
Totals Coverage Status
Change from base Build 1097200852: -0.9%
Covered Lines: 631
Relevant Lines: 712

💛 - Coveralls

Copy link
Member

@tbonfort tbonfort left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, there are a few changes I'm requesting, feel free to reach out if they aren't clear enough.

@thomascoquet
Copy link
Contributor Author

Hey, did the requested changes!
I wanted to make existing code benefit from the streaming hence the awkwardness in the handlers, but I agree it is cleaner to let the existing NewAdapter as is and create another one for streaming.

@tbonfort
Copy link
Member

actually, after having thought about it a bit more, I believe we should internally only rely on the streamAt interface. KeyReaderAt and RegisterAdapter should be marked as deprecated and fall back to wrapping the provided KeyReaderAt as a KeyStreamerAt. Makes sense?

@thomascoquet
Copy link
Contributor Author

Yes, this is what I originally thought as well. But it means the original external API needs to be modified?
It makes sense to go from KeyStreamerAt -> KeyReaderAt; the opposite is less logical.

Copy link
Member

@tbonfort tbonfort left a comment

Choose a reason for hiding this comment

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

I've tried to clarify through these comments. The suggested changes do not modify the existing API, and transparently handles the case when the given reader only implements KeyReaderAt

@tbonfort
Copy link
Member

hmm, the handler.ReadAt methods can't be removed actually to maintain KeyReaderAt interface. They can just panic as they will never be used

@thomascoquet
Copy link
Contributor Author

Made the changes and deprecated all KeyReaderAt methods.
I tried to update the comments regarding error conventions, could use a thorough second look!
The linting is failing because of io.NopCloser on golang 1.15, do you prefer I use ioutil?

@tbonfort tbonfort merged commit 2e02b06 into airbusgeo:main Sep 16, 2021
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.

3 participants