-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-40557: [C++] Use PutObject
request for S3 in OutputStream when only uploading small data
#41564
Conversation
|
From the log output, it seems like the failing CI jobs are not related to this change. Correct me if I am wrong though. Should I rebase (in case the flaky tests are already fixed on main)? |
I think it’s worth rebasing to see? |
Rebased to current main, now waiting for the CI approval again :) |
Hmm, I'm not sure that is ok. Usually, when opening a file for writing, you expect the initial open to fail if the path cannot be written to. I have no idea how much code relies on that, but that's a common expectation due to how filesystems usually work (e.g. when accessing local storage). |
This isn’t guaranteed with the current implementation though? Putting a part, or completing a multipart upload, can fail in various ways? An obvious one would be a checksum failure. |
My point is that if the path cannot be written to, the error happens when opening the file, not later on. |
That is true. I guess the question is if |
The API docs generally do not go into that level of detail. However, it is a general assumption that a filesystem "looks like" a local filesystem API-wise. It is also much more convenient to get an error early, than after you have already "written" multiple megabytes of data to the file. A compromise would be to add a dedicated option in |
We can do that. I would propose that if the optimization is disabled, we directly use multi-part uploads (basically replicating the old behavior). I don't think it makes sense to explicitly issue a HeadBucket request because that will lead to minimum 4 requests with multi-part uploads then. (although we would only have 2 requests for small writes without the optimization compared to current |
That sounds reasonable to me. |
Just to note, issuing I think an optimization flag is appropriate as the behaviour is technically changing. Does it make sense to make the flag a somewhat generic one, rather than specific to this case? There are a few other optimizations that might also fall into the "more performant, slightly different semantics" category. If I was to contribute a PR to improve one of the linked areas, would we want to add a new specific flag for this case or bundle it under a single "optimized" flag? The upside would be that it becomes more configurable, whereas the downside is that the testing and support matrix explodes. Perhaps it's better to just have a single Edit: i guess this is only relevant for higher-level Python bindings, we'd still want internal flags for individual features. |
I added a |
Sorry for delaying review! Would merge after other committers approve |
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.
Thanks for this @OliLay ! I have some questions and suggestions below.
I also merged main into this branch due to a conflict. Should be free of conflicts now. |
Hi @pitrou , I merged main again due to a conflict. Maybe you could give it another review if you have time. |
Gental ping @pitrou . This could be in 18.0.0 (next release) |
@github-actions crossbow submit -g cpp -g python |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g cpp -g python |
Revision: 670232b Submitted crossbow builds: ursacomputing/crossbow @ actions-86d9d85a0e |
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.
+1, thanks a lot for doing this!
@OliLay thank you for your work on this ❤️ |
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 9e6acbe. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
Rationale for this change
See #40557. The previous implementation would always issue multi part uploads which come with 3x RTT to S3 instead of just 1x RTT with a
PutObject
request.What changes are included in this PR?
Implement logic in the S3
OutputStream
to use aPutObject
request if data is below a certain threshold (5 MB) and the output stream is closed. If more data is written, a multi part upload is triggered. Note: Previously, opening the output stream was already expensive because theCreateMultipartUpload
request was triggered then. With this change opening the output stream becomes cheap, as we rather wait until some data is written to decide which upload method to use. This required some more state-keeping in the output stream class.Are these changes tested?
No new tests were added, as there are already tests for very small writes and very large writes, which will trigger both ways of uploading. Everything should therefore be covered by existing tests.
Are there any user-facing changes?
CreateMultipartUpload
request, which we now do not send anymore upon opening the stream. We now rather fail at closing, or at writing (when >5MB have accumulated). Replicating the old behavior is not possible without sending another request which defeats the purpose of this performance optimization. I hope this is fine.