You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "avantgardnerio (via GitHub)" <gi...@apache.org> on 2024/11/25 18:53:27 UTC
[PR] Failing test for multipart uploads with signatures [arrow-rs]
avantgardnerio opened a new pull request, #6794:
URL: https://github.com/apache/arrow-rs/pull/6794
# Which issue does this PR close?
Closes #6793.
# Rationale for this change
Described in issue
# What changes are included in this PR?
A failing test
# Are there any user-facing changes?
Multipart uploads fail
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868525102
##########
object_store/src/aws/client.rs:
##########
@@ -682,6 +716,10 @@ impl S3Client {
response.e_tag
}
};
+
+ let meta = PartMetadata { e_tag, checksum };
+ let content_id = quick_xml::se::to_string(&meta).unwrap();
Review Comment:
This logic should only be enabled if checksums are enabled
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2504805346
> this, an AWS specific detail now leaks into the public trait for all stores... It also is not immediately obvious when the checksums are needed or not...
FWIW: I verified in the new tests we are now writing the checksums at the correct times for when the digest functionality is enabled.
Now I am just trying to get the original tests working for when it isn't.
--
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] Fix multipart uploads with checksums on object locked buckets [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio merged PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868447705
##########
object_store/src/aws/client.rs:
##########
@@ -645,9 +670,13 @@ impl S3Client {
) -> Result<PartId> {
let is_copy = matches!(data, PutPartPayload::Copy(_));
let part = (part_idx + 1).to_string();
+ let config = S3Config {
+ checksum: Some(Checksum::SHA256),
+ ..self.config.clone()
+ };
Review Comment:
Put multipart won't pass on object locked buckets without it, so I figured may as well just always add it, lest we have more plumbing (which is also fine with me) to selectively enable it.
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868233076
##########
object_store/src/aws/client.rs:
##########
@@ -616,9 +637,13 @@ impl S3Client {
&self,
location: &Path,
opts: PutMultipartOpts,
+ checksum: bool,
Review Comment:
Why can't we inspect `self.config`?
--
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] Fix multipart uploads with checksums on object locked buckets [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2525814153
@tustvold some unrelated tests started failing. Do you know what's up?
https://github.com/apache/arrow-rs/actions/runs/12221975779/job/34091689971?pr=6794
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1869393734
##########
object_store/src/client/s3.rs:
##########
@@ -106,14 +106,32 @@ pub(crate) struct CompleteMultipartUpload {
pub part: Vec<MultipartPart>,
}
+#[derive(Serialize, Deserialize)]
+pub(crate) struct PartMetadata {
+ pub e_tag: String,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub checksum: Option<String>,
Review Comment:
Do we want to add algorithm here?
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1869392745
##########
object_store/src/aws/client.rs:
##########
@@ -669,8 +672,13 @@ impl S3Client {
request = request.with_encryption_headers();
}
let response = request.send().await?;
+ let checksum = response
+ .headers()
+ .get(SHA256_CHECKSUM)
+ .and_then(|v| v.to_str().ok())
Review Comment:
Do we need to unwind these errors or swallow them?
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2508919453
So I think there is an less intrusive way to support this.
UploadPart is not an issue as the per-part meta is never exposed.
The issue solely concerns MultipartStore, in particular how to carry the checksum through https://docs.rs/object_store/latest/object_store/multipart/struct.PartId.html.
Now we could add a checksum field to this, but this would be a breaking change. However, the content id is intentionally opaque. We could therefore just use the XML encoded https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompletedPart.html instead of an ETag when necessary.
There would need to still be logic to handle an ETag based PartId, as these may have been stored in a database, but when checksums, but detecting this should be trivial e.g. by attempting to decode the XML.
This would avoid needing to make any breaking changes, whilst also being future proof for any of the other checksum types
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868523788
##########
object_store/src/aws/client.rs:
##########
@@ -616,9 +637,13 @@ impl S3Client {
&self,
location: &Path,
opts: PutMultipartOpts,
+ checksum: bool,
Review Comment:
Why does it need to be false?
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1869393734
##########
object_store/src/client/s3.rs:
##########
@@ -106,14 +106,32 @@ pub(crate) struct CompleteMultipartUpload {
pub part: Vec<MultipartPart>,
}
+#[derive(Serialize, Deserialize)]
+pub(crate) struct PartMetadata {
+ pub e_tag: String,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub checksum: Option<String>,
Review Comment:
Do we want to add algorithm here? If so, how? As the raw enum? Should it then implement `Serialize` as well?
--
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] Fix multipart uploads with checksums on object locked buckets [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1870958271
##########
object_store/src/aws/client.rs:
##########
@@ -617,8 +617,11 @@ impl S3Client {
location: &Path,
opts: PutMultipartOpts,
) -> Result<MultipartId> {
- let response = self
- .request(Method::POST, location)
+ let mut reqquest = self.request(Method::POST, location);
+ if let Some(algorithm) = self.config.checksum {
+ reqquest = reqquest.header(ALGORITHM, &algorithm.to_string().to_uppercase());
Review Comment:
* AFAIK you can do `algorithm.to_uppercase()` to avoid one allocation
* I think it's better to match against `algorithm` to create a `SHA256`
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868231890
##########
object_store/src/aws/client.rs:
##########
@@ -645,9 +670,13 @@ impl S3Client {
) -> Result<PartId> {
let is_copy = matches!(data, PutPartPayload::Copy(_));
let part = (part_idx + 1).to_string();
+ let config = S3Config {
+ checksum: Some(Checksum::SHA256),
+ ..self.config.clone()
+ };
Review Comment:
Why 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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2498996306
So there are a couple of issues with how multi-part upload is implemented:
1. The initial `PUT` request to `CreateMultipartUpload` must specify the checksum algo or else calling `PutPart` with the checksum will fail
2. When calling `CompleteMultipartUpload` we need to send all the part checksums in the request (see `https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html)
So this means that we need to keep the checksums in the `PartId` for when we complete the upload
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2506306684
> The current implementation does 2 but not 1 or 3.
I'd like to re-iterate that this PR is functionally complete. Although I am unsure about the docs, I am sure it works.
Now I'm just trying to fix the existing `s3_test`.
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2506430691
> Was just trying to clarify for @tustvold based on his earlier comment
Right, my comment pertained to the changes to the public API, we now have a somewhat peculiar quirk of AWS leaking out into the public API in a manner that also makes it very unclear when or if I need to specify the checksum or not. I need to find some time to sit down and think about how we could support this, ideally without making a breaking API 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] Fix multipart uploads with checksums on object locked buckets [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1874762133
##########
object_store/src/client/s3.rs:
##########
@@ -106,14 +106,32 @@ pub(crate) struct CompleteMultipartUpload {
pub part: Vec<MultipartPart>,
}
+#[derive(Serialize, Deserialize)]
+pub(crate) struct PartMetadata {
+ pub e_tag: String,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub checksum: Option<String>,
Review Comment:
```suggestion
pub checksum_sha256: Option<String>,
```
##########
object_store/src/aws/client.rs:
##########
@@ -617,8 +617,15 @@ impl S3Client {
location: &Path,
opts: PutMultipartOpts,
) -> Result<MultipartId> {
- let response = self
- .request(Method::POST, location)
+ let mut reqquest = self.request(Method::POST, location);
Review Comment:
```suggestion
let mut request = self.request(Method::POST, location);
```
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2504792466
I'm not sure about this, an AWS specific detail now leaks into the public trait for all stores... It also is not immediately obvious when the checksums are needed or not...
I think this needs a bit more thought, I'll try to find some time in the next few weeks
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1862288356
##########
object_store/src/multipart.rs:
##########
@@ -21,18 +21,36 @@
//! cloud storage services. It's designed to offer efficient, non-blocking operations,
//! especially useful when dealing with large files or high-throughput systems.
-use async_trait::async_trait;
-
use crate::path::Path;
use crate::{MultipartId, PutPayload, PutResult, Result};
+use async_trait::async_trait;
-/// Represents a part of a file that has been successfully uploaded in a multipart upload process.
+/// An etag of a part of a file that has been successfully uploaded in a multipart upload process.
#[derive(Debug, Clone)]
pub struct PartId {
/// Id of this part
pub content_id: String,
}
+/// Represents a part of a file that has been successfully uploaded in a multipart upload process.
+#[derive(Debug)]
+pub struct MultipartInfo {
+ /// Id of this part
+ pub e_tag: String,
+ /// Index of the part + 1
+ pub part_number: usize,
+ /// 256 bit SHA of the contents
+ pub checksum_sha256: Option<String>,
Review Comment:
Probably shouldn't assume this is a `sha256` checksum. Maybe just rename to `checksum` and let it be opaque.
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1869395306
##########
object_store/src/aws/client.rs:
##########
@@ -682,6 +690,14 @@ impl S3Client {
response.e_tag
}
};
+
+ let content_id = if self.config.checksum == Some(Checksum::SHA256) {
+ let meta = PartMetadata { e_tag, checksum };
Review Comment:
For future proofing, we might want to set `PartMetadata::algorithm`...
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1869385185
##########
object_store/src/aws/client.rs:
##########
@@ -616,9 +637,13 @@ impl S3Client {
&self,
location: &Path,
opts: PutMultipartOpts,
+ checksum: bool,
Review Comment:
I was wrong. This code has been removed.
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2515326804
> We could therefore just use the XML encoded
@tustvold done
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2506302158
> It also is not immediately obvious when the checksums are needed or not...
The AWS docs are not very helpful here but we pieced together that to write to an S3 bucket using multi-part with object locking enabled you need to:
1. Specify the checksum algo in the initial API call to `CreateMultiPartUpload`
2. Send the checksum for each part while calling `PutPart`
3. When calling `CompleteMultipartUpload` you need to send the checksum again for each part that was uploaded
The current implementation does 2 but not 1 or 3.
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "thinkharderdev (via GitHub)" <gi...@apache.org>.
thinkharderdev commented on PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#issuecomment-2506315187
> > The current implementation does 2 but not 1 or 3.
>
> I'd like to re-iterate that this PR is functionally complete. Although I am unsure about the docs, I am sure it works.
>
> Now I'm just trying to fix the existing `s3_test`.
Sorry, by current implementation I meant what is on `master` not what is in this PR (which does it correctly) 😄
Was just trying to clarify for @tustvold based on his earlier comment
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868450453
##########
object_store/src/aws/client.rs:
##########
@@ -616,9 +637,13 @@ impl S3Client {
&self,
location: &Path,
opts: PutMultipartOpts,
+ checksum: bool,
Review Comment:
Because it needs to be `false` here: https://github.com/apache/arrow-rs/blob/9047d99f6bf87582532ee6ed0acb3f2d5f889f11/object_store/src/aws/mod.rs#L318
But true for the other calls. If we want to do something like `self.client.clone_with_config(&new_config)` that would work too...
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868524481
##########
object_store/src/aws/client.rs:
##########
@@ -645,9 +670,13 @@ impl S3Client {
) -> Result<PartId> {
let is_copy = matches!(data, PutPartPayload::Copy(_));
let part = (part_idx + 1).to_string();
+ let config = S3Config {
+ checksum: Some(Checksum::SHA256),
+ ..self.config.clone()
+ };
Review Comment:
Surely we should only specify it if checksums have been enabled on the builder?
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1868530053
##########
object_store/src/aws/client.rs:
##########
@@ -616,9 +637,13 @@ impl S3Client {
&self,
location: &Path,
opts: PutMultipartOpts,
+ checksum: bool,
Review Comment:
When you're uploading parts, it seems to want it set to the checksum of the data in the part. When you're doing a copy, it does not
--
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] Failing test for multipart uploads with checksums [arrow-rs]
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1869392150
##########
object_store/src/aws/client.rs:
##########
@@ -617,8 +617,11 @@ impl S3Client {
location: &Path,
opts: PutMultipartOpts,
) -> Result<MultipartId> {
- let response = self
- .request(Method::POST, location)
+ let mut reqquest = self.request(Method::POST, location);
+ if let Some(algorithm) = self.config.checksum {
+ reqquest = reqquest.header(ALGORITHM, &algorithm.to_string().to_uppercase());
Review Comment:
Do we want to rely on `to_uppercase` here?
--
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] Fix multipart uploads with checksums on object locked buckets [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #6794:
URL: https://github.com/apache/arrow-rs/pull/6794#discussion_r1870958271
##########
object_store/src/aws/client.rs:
##########
@@ -617,8 +617,11 @@ impl S3Client {
location: &Path,
opts: PutMultipartOpts,
) -> Result<MultipartId> {
- let response = self
- .request(Method::POST, location)
+ let mut reqquest = self.request(Method::POST, location);
+ if let Some(algorithm) = self.config.checksum {
+ reqquest = reqquest.header(ALGORITHM, &algorithm.to_string().to_uppercase());
Review Comment:
* AFAIK you can do `algorithm.to_uppercase()` to avoid one allocation
* I think it's better to match against `algorithm` to create a `SHA256`.
--
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