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

Add byte range support to OpenAPI (server, spec and generated clients) #4623

Merged
merged 5 commits into from
Nov 22, 2022

Conversation

ozkatz
Copy link
Collaborator

@ozkatz ozkatz commented Nov 21, 2022

Context: trying to read parquet footer from Pandas using the Python SDK, I discovered I can't read the Parquet footer since the API does not support byte ranges

@ozkatz ozkatz added area/API Improvements or additions to the API minor-change Used for PRs that don't require issue attached labels Nov 21, 2022
@ozkatz ozkatz requested a review from nopcoder November 21, 2022 13:37
@ozkatz ozkatz self-assigned this Nov 21, 2022
@ozkatz ozkatz changed the title added byte range support to OpenAPI add byte range support to OpenAPI (server, spec and generated clients) Nov 21, 2022
@ozkatz ozkatz added include-changelog PR description should be included in next release changelog and removed minor-change Used for PRs that don't require issue attached labels Nov 21, 2022
@ozkatz
Copy link
Collaborator Author

ozkatz commented Nov 21, 2022

closes #4624

@ozkatz ozkatz linked an issue Nov 21, 2022 that may be closed by this pull request
@treeverse treeverse deleted a comment from github-actions bot Nov 21, 2022
@ozkatz ozkatz marked this pull request as ready for review November 21, 2022 13:49
pkg/api/controller.go Outdated Show resolved Hide resolved
pkg/httputil/range_test.go Outdated Show resolved Hide resolved
@nopcoder nopcoder changed the title add byte range support to OpenAPI (server, spec and generated clients) Add byte range support to OpenAPI (server, spec and generated clients) Nov 21, 2022
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

lgtm - require an issue to cover by test

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Neat stuff! I think we had a user request for this a while back, but couldn't find it.

Comment on lines +2710 to +2727
headers:
Content-Length:
schema:
type: integer
format: int64
Content-Range:
schema:
type: string
pattern: '^bytes=((\d*-\d*,? ?)+)$'
Last-Modified:
schema:
type: string
ETag:
schema:
type: string
Content-Disposition:
schema:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can easily share the schemata for the parts shared with 200, it would be great. But not worth making a great effort for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really sure how to do that.. I can try in a different PR (but not guaranteeing anything :))

t.Fatal(err)
}
if resp.HTTPResponse.StatusCode != http.StatusPartialContent {
t.Fatalf("GetObject() status code %d, expected %d", resp.HTTPResponse.StatusCode, http.StatusPartialContent)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can make this and most or all following errors t.Errorf, the added error messages might greatly aid debugging if/when this test fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Changed this and adjacent get_object tests to avoid scope creep for this PR.. we might want to somehow lint for this..

@ozkatz ozkatz merged commit fc4e1c2 into master Nov 22, 2022
@ozkatz ozkatz deleted the feature/add-range-to-openapi branch November 22, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lakeFS OpenAPI does not support byte ranges
3 participants