Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Wrap S3 multipart upload exception #31195

Merged
merged 2 commits into from
Feb 21, 2022
Merged

Wrap S3 multipart upload exception #31195

merged 2 commits into from
Feb 21, 2022

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Feb 15, 2022 •
edited
Loading

The returned HTTP code is now 502 instead of 500 in case of timeout during S3 bucket upload.

Hopefully, this would help unofficial clients to deal with this kind of error in a better way.

@artonge
Wrap S3 multipart upload exception
0b768c1 
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge added enhancement 3. to review Waiting for reviews labels Feb 15, 2022
@artonge artonge added this to the Nextcloud 24 milestone Feb 15, 2022
@artonge artonge self-assigned this Feb 15, 2022
@artonge artonge closed this Feb 15, 2022
@artonge artonge reopened this Feb 16, 2022
PVince81
PVince81 approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks fine

I'm not 100% sure about "502 bad gateway" even though technically Nextcloud is like a proxy here. Let's see what the second reviewer thinks

@artonge
Copy link
Contributor Author

artonge commented Feb 16, 2022 •
edited
Loading

I'm not 100% sure about "502 bad gateway" even though technically Nextcloud is like a proxy here. Let's see what the second reviewer thinks

Me neither, I would prefer the timeout code (408) but I didn't find a way to check if the error was from a timeout or something else.

But even the timeout error code would not reflect the error correctly.

@artonge
Wrap S3 multipart upload exception
bc2c02f 
Signed-off-by: Louis Chemineau <louis@chmn.me>
ChristophWurst
ChristophWurst approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🐘

@artonge
Copy link
Contributor Author

artonge commented Feb 21, 2022

CI failure unrelated

@artonge artonge merged commit d721339 into master Feb 21, 2022
@artonge artonge deleted the wrap_exception branch February 21, 2022 10:27
@PVince81
Copy link
Member

/backport to stable23

@PVince81
Copy link
Member

/backport to stable22

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 21, 2022
Merged
@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees

@artonge artonge

Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.
3 participants
@artonge @PVince81 @ChristophWurst