You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jkosh44 (via GitHub)" <gi...@apache.org> on 2025/03/20 21:37:49 UTC
[PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
jkosh44 opened a new pull request, #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257
# Which issue does this PR close?
None
Closes #.
# Rationale for this change
Fixes unreported error
# What changes are included in this PR?
This commit fixes the GCP mulipart complete implementation by adding the Content-Length header to the PUT XML requests that happens when the completed parts are empty. GCP is strict about setting the Content-Length header on requests with empty bodies, so previously this would result in a 411 error.
# Are there any user-facing changes?
Yes, empty multipart uploads don't cause 411 in GCP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007245772
##########
src/gcp/client.rs:
##########
@@ -517,6 +517,7 @@ impl GoogleCloudStorageClient {
// GCS doesn't allow empty multipart uploads
let result = self
.request(Method::PUT, path)
+ .header(&CONTENT_LENGTH, "0")
.idempotent(true)
.do_put()
.await?;
Review Comment:
```suggestion
let result = self.put(path, PutPayload::new(), Default::default()).await?;
```
This will help to avoid any potential divergence in future, and also perhaps be a bit more clear what is going on?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2749108584
Thank you for the reviews @alamb and @tustvold!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "andrew-pienso (via GitHub)" <gi...@apache.org>.
andrew-pienso commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2749254089
Thank you all!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2741788557
@alamb I wasn't able to come up with a good unit test for this.l
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007447299
##########
src/gcp/client.rs:
##########
@@ -517,6 +517,7 @@ impl GoogleCloudStorageClient {
// GCS doesn't allow empty multipart uploads
let result = self
.request(Method::PUT, path)
+ .header(&CONTENT_LENGTH, "0")
.idempotent(true)
.do_put()
.await?;
Review Comment:
I've updated to do this, but I also had to update the implementation of `self.put` to set `Content-Length` for empty payloads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2741941627
Thanks @jkosh44 -- I am going to wait to merge this PR for a day or so to let some of the other maintainers double check I got this new repo set up correctly, but I think this PR is ready to merge from my perspective
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "andrew-pienso (via GitHub)" <gi...@apache.org>.
andrew-pienso commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2749370140
> > Thank you all!
>
> @andrew-pienso if you still have a repro of #278, it would definitely be helpful to double check that this does in fact resolve your issue.
When I filed this bug, this [example here would repro](https://github.com/delta-io/delta-rs/issues/2394#issuecomment-2132886429). I played around with this for a good half hour and am admittedly not able to repro this anymore in my environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2749293338
> Thank you all!
@andrew-pienso if you still have a repro of https://github.com/apache/arrow-rs-object-store/issues/278, it would definitely be helpful to double check that this does in fact resolve your issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2008776349
##########
src/gcp/client.rs:
##########
@@ -516,9 +516,7 @@ impl GoogleCloudStorageClient {
if completed_parts.is_empty() {
// GCS doesn't allow empty multipart uploads
let result = self
- .request(Method::PUT, path)
- .idempotent(true)
- .do_put()
+ .put(path, PutPayload::new(), Default::default())
.await?;
self.multipart_cleanup(path, multipart_id).await?;
Review Comment:
@tustvold I was also trying to think about how to make this more obvious. Perhaps the following structure is more clear? The comment clarifies that we're falling back to a non-multipart upload. Also moving the PUT after the cleanup makes it clear that the cleanup is not affecting the PUT. What do you think?
```rust
// GCS doesn't allow empty multipart uploads, so fallback to regular upload.
self.multipart_cleanup(path, multipart_id).await?;
let result = self
.put(path, PutPayload::new(), Default::default())
.await?;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#issuecomment-2748989864
Thank you @jkosh44 -- this is the first PR in the new repo to merge with feature change. So exciting!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007449981
##########
src/gcp/client.rs:
##########
@@ -397,8 +397,13 @@ impl GoogleCloudStorageClient {
extensions,
} = opts;
- let builder = self
- .request(Method::PUT, path)
+ let builder = self.request(Method::PUT, path);
+ let builder = if payload.content_length() == 0 {
+ builder.header(&CONTENT_LENGTH, "0")
+ } else {
+ builder
+ };
+ let builder = builder
Review Comment:
This is unnecessary, with_payload already handles this
##########
src/gcp/client.rs:
##########
@@ -397,8 +397,13 @@ impl GoogleCloudStorageClient {
extensions,
} = opts;
- let builder = self
- .request(Method::PUT, path)
+ let builder = self.request(Method::PUT, path);
+ let builder = if payload.content_length() == 0 {
+ builder.header(&CONTENT_LENGTH, "0")
+ } else {
+ builder
+ };
+ let builder = builder
Review Comment:
This change is unnecessary, with_payload already handles this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007452159
##########
src/gcp/client.rs:
##########
@@ -517,6 +517,7 @@ impl GoogleCloudStorageClient {
// GCS doesn't allow empty multipart uploads
let result = self
.request(Method::PUT, path)
+ .header(&CONTENT_LENGTH, "0")
.idempotent(true)
.do_put()
.await?;
Review Comment:
Put already handles this, this is why we don't see this issue with regular empty non-multipart uploads
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007507428
##########
src/gcp/client.rs:
##########
@@ -397,8 +397,13 @@ impl GoogleCloudStorageClient {
extensions,
} = opts;
- let builder = self
- .request(Method::PUT, path)
+ let builder = self.request(Method::PUT, path);
+ let builder = if payload.content_length() == 0 {
+ builder.header(&CONTENT_LENGTH, "0")
+ } else {
+ builder
+ };
+ let builder = builder
Review Comment:
Ah got it, I just reverted this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2008841349
##########
src/gcp/client.rs:
##########
@@ -516,9 +516,7 @@ impl GoogleCloudStorageClient {
if completed_parts.is_empty() {
// GCS doesn't allow empty multipart uploads
let result = self
- .request(Method::PUT, path)
- .idempotent(true)
- .do_put()
+ .put(path, PutPayload::new(), Default::default())
.await?;
self.multipart_cleanup(path, multipart_id).await?;
Review Comment:
Ok, I made the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44-datadog (via GitHub)" <gi...@apache.org>.
jkosh44-datadog commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007503790
##########
src/gcp/client.rs:
##########
@@ -397,8 +397,13 @@ impl GoogleCloudStorageClient {
extensions,
} = opts;
- let builder = self
- .request(Method::PUT, path)
+ let builder = self.request(Method::PUT, path);
+ let builder = if payload.content_length() == 0 {
+ builder.header(&CONTENT_LENGTH, "0")
+ } else {
+ builder
+ };
+ let builder = builder
Review Comment:
Ah got it, I just reverted this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "jkosh44-datadog (via GitHub)" <gi...@apache.org>.
jkosh44-datadog commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2007503790
##########
src/gcp/client.rs:
##########
@@ -397,8 +397,13 @@ impl GoogleCloudStorageClient {
extensions,
} = opts;
- let builder = self
- .request(Method::PUT, path)
+ let builder = self.request(Method::PUT, path);
+ let builder = if payload.content_length() == 0 {
+ builder.header(&CONTENT_LENGTH, "0")
+ } else {
+ builder
+ };
+ let builder = builder
Review Comment:
Ah got it, I just reverted this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add content length to PUT GCP multipart complete [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #257:
URL: https://github.com/apache/arrow-rs-object-store/pull/257#discussion_r2008834674
##########
src/gcp/client.rs:
##########
@@ -516,9 +516,7 @@ impl GoogleCloudStorageClient {
if completed_parts.is_empty() {
// GCS doesn't allow empty multipart uploads
let result = self
- .request(Method::PUT, path)
- .idempotent(true)
- .do_put()
+ .put(path, PutPayload::new(), Default::default())
.await?;
self.multipart_cleanup(path, multipart_id).await?;
Review Comment:
I don't feel strongly either way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org