Skip to content

Conversation

@mzamot
Copy link
Contributor

@mzamot mzamot commented Apr 8, 2019

After testing PR #14570 in NC15 and Master, I couldn't write files bigger than 10MB with the negation on the verification. After removing the negation it worked just fine.

See issue #13554

Signed-off-by: Michael Zamot <michael@zamot.io>
icewind1991
icewind1991 previously approved these changes Apr 9, 2019
@icewind1991 icewind1991 dismissed their stale review April 9, 2019 16:02

read it wrong

@icewind1991
Copy link
Member

Removing the negation here is incorrect, since it would then read use the logic for non-seekable streams for streams that are seekable and not for streams that aren't. The issue here is that streams which are seekable do not upload correctly, that either needs to be resolved or the entire check needs to be removed and we always need to do the fallback code

@schoenpat
Copy link

@icewind1991
Even if the fix isn't good, it works. Since 15.0.0 this bug exists and i'm tired of fixing it manually after every update by applying a patch. Why not accepting this pr so that in the next version swift storage is fixed and then there is enough time to develop a more beautifull fix?

@kesselb
Copy link
Collaborator

kesselb commented Apr 16, 2019

the entire check needs to be removed and we always need to do the fallback code

@schoenpat this would be an acceptable "fix". changing the condition like suggested is wrong. should be easy to change this pr like suggested @mzamot 🏓

@rullzer
Copy link
Member

rullzer commented Apr 18, 2019

I remove the entire check in #15168.
So we can make sure it lands in 16.

@rullzer rullzer closed this Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants