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.

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

Reset blacklist attempt counter when chunks succeeded after failed attempts #3934

Closed
PVince81 opened this issue Oct 8, 2015 · 17 comments

Comments

@PVince81
Copy link
Member

commented Oct 8, 2015

@guruz: I had a chat with @stonerl on IRC and he told me that he had a resumable situation. Basically chunks fail from times to times but the second time it works. But if within the same transfer several chunks failed, the file still gets blacklisted. @guruz does that match the current implementation of the client ?

If yes I'd say the client should discard the blacklist counter value as soon as there is a successful chunk. Only if the same chunk fails over and over again, then blacklist the file.
Unless you are able to track on a chunk to chunk basis instead of transfer basis, but that might be overkill.

@guruz guruz added this to the 2.1-next milestone Oct 8, 2015

@guruz guruz added the Design & UX label Oct 8, 2015

@guruz guruz modified the milestones: 2.2-next, 2.1-next Oct 8, 2015

@ckamm

This comment has been minimized.

Copy link
Member

commented Oct 15, 2015

Doing this seems very easy, just call journal->wipeErrorBlacklistEntry() when a chunk downloaded successfully.

@PVince81

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2015

Just to clarify, this was an upload situation, not download.
So you mean "uploaded" ?

guruz added a commit that referenced this issue Oct 16, 2015

@guruz guruz modified the milestones: 2.0.2-current, 2.2-next Oct 16, 2015

@guruz

This comment has been minimized.

Copy link
Collaborator

commented Oct 16, 2015

Done for uploads for 2.0.2

Although @ogoffart is worried that it voids the use of a blacklist in the first place.. let's see.

@guruz guruz closed this Oct 16, 2015

@PVince81

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2015

The blacklist would still kick in if a single chunk could not be uploaded several times due to network errors. But yeah, it might happen less often.

@stonerl

This comment has been minimized.

Copy link

commented Oct 16, 2015

Is this in the current nightly? If so I'm going to test this.

@guruz

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2015

@stonerl should be, please test :)

@guruz guruz reopened this Oct 19, 2015

@guruz guruz added the ReadyToTest label Oct 19, 2015

@guruz

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2015

@stonerl And by current I mean please test the one from today (19th)

@guruz guruz self-assigned this Oct 19, 2015

@PVince81

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2015

2.0.2 is out already, is this fix in ?

@guruz

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2015

@PVince81 yes, see milestone and commit above

@dragotin dragotin modified the milestones: 2.1-current, 2.0.2 Nov 11, 2015

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 13, 2015

@stonerl could you install this version: http://download.owncloud.com/desktop/daily/ownCloud-2.1.0.2875-nightly20151113.pkg, please
And try again
Thanks

@stonerl

This comment has been minimized.

Copy link

commented Nov 13, 2015

Does not work. The second failed chunk still blacklists the file, even if other chunks have been uploaded successfully.

Error downloading https://example.tld/remote.php/webdav/Documents/cm-12.1-20151103-NIGHTLY-bacon-1.zip-chunking-1852184230-60-6 - server replied: Bad Request (expected filesize 5242880 got 0)

Continue blacklisting: Error downloading https://example.tld/remote.php/webdav/Documents/cm-12.1-20151103-NIGHTLY-bacon-1.zip-chunking-1852184230-60-9 - server replied: Bad Request (expected filesize 5242880 got 0)

@Dianafg76 Dianafg76 removed the ReadyToTest label Nov 16, 2015

@guruz

This comment has been minimized.

Copy link
Collaborator

commented Nov 17, 2015

@stonerl Wait, you mean even if a chunk after a failed chunk suceeds?

@stonerl

This comment has been minimized.

Copy link

commented Nov 17, 2015

@guruz

Yes, chunk 6 failed first. The client continued uploading later and was able to upload chunk 6 and 7. While it was uploading chunk 8 and 9 chunk 9 failed and the file got blacklisted (as you can see from the activity-log).

@ckamm ckamm unassigned guruz Nov 24, 2015

@ckamm ckamm self-assigned this Nov 24, 2015

@ckamm ckamm added ReadyToTest and removed Needs info labels Nov 24, 2015

@ckamm

This comment has been minimized.

Copy link
Member

commented Nov 24, 2015

@stonerl The remaining problem was about error reporting. The blacklist entry was correctly wiped and you got a fresh 25s blacklist entry the second time a chunk failed - but got the "Continue blacklisting" message. With the new patch this should be better.

@stonerl

This comment has been minimized.

Copy link

commented Nov 24, 2015

OK, thank you for clarification.

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 25, 2015

@stonerl

This comment has been minimized.

Copy link

commented Nov 29, 2015

👍 works great. Thanks to all of you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.