ARROW-14523: [C++] Fix potential data loss in S3 multipart upload #11594
Conversation
|
|
There was a problem hiding this comment.
Looks good. I notice their own workaround looked for a case where the root element was correct but the first child was Error which isn't handled here. I'm not sure what they were after and I couldn't find any documentation that such a response was possible so I think what you have is correct but figured I'd mention it in case you'd seen something like that before.
| stream.clear(); | ||
| stream.seekg(pos); |
There was a problem hiding this comment.
Nit: I think this is redundant since you cleared above.
There was a problem hiding this comment.
GetErrorMarshaller()->Marshall will involve XML parsing over the response stream again.
|
@github-actions crossbow submit wheel |
|
Revision: 9ca9fd4 Submitted crossbow builds: ursacomputing/crossbow @ actions-1076
|
Hmm, I may try to detect that indeed. |
| long long) { // NOLINT runtime/int | ||
| auto& stream = http_resp->GetResponseBody(); | ||
| const auto pos = stream.tellg(); | ||
| const auto doc = Aws::Utils::Xml::XmlDocument::CreateFromXmlStream(stream); |
There was a problem hiding this comment.
I know it's possible to get a partial XML response from S3 due to network failure, does this handle that and throw a 5xx/retry or will an XML error be thrown back up? https://github.com/boto/botocore/blob/04d1fae43b657952e49b21d16daa86378ddb4253/botocore/handlers.py#L141
Per AWS:
# In cases of network disruptions, we may end up with a partial
# streamed response from S3. We need to treat these cases as
# 500 Service Errors and try again.
There was a problem hiding this comment.
This should be normally detected as an error by the AWS SDK, so no need to special-case this on our own (hopefully).
pitrou
force-pushed
the
ARROW-14523-s3-cmu-error-fix
branch
from
9ca9fd4 to
958dc16
Compare
|
@github-actions crossbow submit wheel--cp39- |
|
Revision: 958dc16 Submitted crossbow builds: ursacomputing/crossbow @ actions-1078
|
|
@seittema Could you try out one the wheels produced just above? You'll find the download URL in the "upload artifacts" step of the corresponding build. |
|
@pitrou could you please remove the excessive logging so we can cut the first release candidate for 6.0.1? We can address additional problems during the verification process and create a new release candidate if required. |
|
Will do. |
Work around a critical bug in the AWS SDK for C++ that fails to detect errors returned by CompleteMultipartUpload in the body of a 200 OK response: aws/aws-sdk-cpp#658
pitrou
force-pushed
the
ARROW-14523-s3-cmu-error-fix
branch
from
958dc16 to
87f7573
Compare
|
Benchmark runs are scheduled for baseline = 0ead7c9 and contender = 3626a08. 3626a08 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Work around a critical bug in the AWS SDK for C++ that fails to detect errors returned by CompleteMultipartUpload in the body of a 200 OK response: aws/aws-sdk-cpp#658 Closes #11594 from pitrou/ARROW-14523-s3-cmu-error-fix Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
Work around a critical bug in the AWS SDK for C++ that fails to detect errors returned by CompleteMultipartUpload in the body of a 200 OK response:
aws/aws-sdk-cpp#658