fix(s3): make leavePartsOnError default to true #4414
Conversation
| @@ -21,7 +21,7 @@ try { | |||
| ], // optional tags | |||
| queueSize: 4, // optional concurrency configuration | |||
| partSize: 1024 * 1024 * 5, // optional size of each part, in bytes, at least 5MB | |||
| leavePartsOnError: false, // optional manually handle dropped parts | |||
| leavePartsOnError: true, // optional throw error on failed parts | |||
There was a problem hiding this comment.
I think this flag name is confusing. I'm bad with naming but maybe change it to something like
| leavePartsOnError: true, // optional throw error on failed parts | |
| throwOnPartsError: true, // optional throw error on failed parts |
There was a problem hiding this comment.
I agree it's a bad name, but that would be an even more breaking change. maybe a todo for the future?
|
Hi there @mifi , As mentioned in the comments, changing the name for this config type would be considered a breaking change and not something we can merge. Can you amend the PR to be a documentation change only? Thanks, |
RanVaknin
added
the
response-requested
|
Hi. I didn't change the name of the variable, just the default value from false to true. do you also consider that a breaking change? I would argue it's not, and that it's rather a bug fix because the default value false leads to such a broken behaviour. |
github-actions
bot
removed
the
response-requested
|
Hi @mifi , The change in behavior would be considered a breaking change since certain customers might rely on the "incorrect" behavior. I'll need to discuss this with the team once again to see if we are willing to accept this and cause a "breaking fix". Thanks, |
|
closing this since a larger fix is needed, will be done in #6112 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue
Issue number, if available, prefixed with "#"
#2311
Description
Changes the unsafe default
leavePartsOnErrorto true. The default would leave the upload corrupt without anyone knowing.Testing
How was this change tested?
Additional context
Add any other context about the PR here.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.