-
Notifications
You must be signed in to change notification settings - Fork 869
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
Handle empty S3 payloads (#4514) #4518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me -- thank you @tustvold
I am just verifying the test coverage... Hang tight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, but I am not sure about the test
@@ -1258,6 +1258,15 @@ mod tests { | |||
} | |||
|
|||
delete_fixtures(storage).await; | |||
|
|||
let path = Path::from("empty"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test appears to pass even without the code changes in this PR. See #4519 for example
You need to test against real S3, the emulator doesn't seem to exhibit the same issue |
I will try |
@tustvold can you give me the command you used run the tests against a real S3 bucket (obviously I will supply my own AWS credentials) I got this far (but then I stumbled on what the "metadata endpoint" should be
|
Just omit the endpoint, note also that the credential names should be prefixed by OBJECT_STORE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I ran the new test against an actual S3 bucket and without the code changes in this PR it fails like this:
---- aws::tests::s3_test stdout ----
thread 'aws::tests::s3_test' panicked at 'called `Result::unwrap()` on an `Err` value: Generic { store: "S3", source: Error { retries: 0, message: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<Error><Code>MissingContentLength</Code><Message>You must provide the Content-Length HTTP header.</Message><RequestId>....</RequestId><HostId>....</HostId></Error>", source: Some(reqwest::Error { kind: Status(411), url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("s3.us-east-1.amazonaws.com")), port: None, path: "/alamb-test-bucket/empty", query: None, fragment: None } }), status: Some(411) } }', src/lib.rs:1263:48
With this PR it passes
Nice work @tustvold -- thank you
Which issue does this PR close?
Closes #4514
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?