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

[MRG+1] Added: Retrying scrapy.core.downloader.handlers.http11.TunnelError #1974

Merged
merged 1 commit into from Jun 6, 2016

Conversation

@starrify
Copy link
Contributor

@starrify starrify commented May 9, 2016

It's a small change. Please just check the diff.

@redapple
Copy link
Contributor

@redapple redapple commented May 9, 2016

hi @starrify , what kind of error triggered TunnelError in your tests?
did you see retrying them succeed eventually?

@starrify
Copy link
Contributor Author

@starrify starrify commented May 9, 2016

Hi @redapple , here is the error message:

[scrapy.core.scraper] Error downloading <GET XXXX>: Could not open CONNECT tunnel.

For details, please check https://staging.scrapinghub.com/p/6327/job/9/17618/#/log?filterAndHigher&filterType=error (private access only). From the job stats we know there's no retrying on such failures.

Here's another sample job after applying the same fix in this PR: https://staging.scrapinghub.com/p/6327/job/9/17622/#details (private access only) where the same error has almost disapeared. Also the job stats suggest that there are many retries.

@redapple
Copy link
Contributor

@redapple redapple commented May 17, 2016

@starrify , could this be configured via a new downloader setting?

@starrify
Copy link
Contributor Author

@starrify starrify commented May 17, 2016

I'm afraid not, since this EXCEPTIONS_TO_RETRY thing is initialized here in this file and never updated later.

However I think it might be a good idea to make it configurable through settings. :)

@redapple
Copy link
Contributor

@redapple redapple commented May 17, 2016

@starrify , I'm not sure I understand your comment.
one could still update an instance attribute with exceptions to retry, defaulting to EXCEPTIONS_TO_RETRY, adding TunnelError to the tuple if some new RETRY_TUNNEL_ERROR is True or something.
maybe I'm missing something?

@codecov-io
Copy link

@codecov-io codecov-io commented May 17, 2016

Current coverage is 83.27%

Merging #1974 into master will increase coverage by <.01%

Powered by Codecov. Last updated by ed4b9af...81ed956

@starrify
Copy link
Contributor Author

@starrify starrify commented May 17, 2016

Hi, seems I'm the one who misunderstood. :( I thought you asked could this be configured now via a downloader setting?

I have updated this PR by introducing a new setting RETRY_EXCEPTIONS_EXTRA and removed TunnelError from the default ones.

But personally I still think TunnelError shall be retried by default like other errors.

@redapple
Copy link
Contributor

@redapple redapple commented May 18, 2016

@starrify , after some thought, I think your initial commit (adding TunnelError to EXCEPTIONS_TO_RETRY) makes more sense in the end. Sorry for making you update it.

@starrify starrify force-pushed the starrify:more-errors-to-retry branch from 81ed956 to 334ee40 May 18, 2016
@starrify
Copy link
Contributor Author

@starrify starrify commented May 18, 2016

np :)

I guess making TunnelError default and introducing a configurable EXCEPTIONS_TO_RETRY are both good things to do. But one thing at a time, and I've just discarded the 2nd commit in this PR.

@redapple redapple changed the title Added: Retrying scrapy.core.downloader.handlers.http11.TunnelError [MRG+1] Added: Retrying scrapy.core.downloader.handlers.http11.TunnelError Jun 6, 2016
@redapple
Copy link
Contributor

@redapple redapple commented Jun 6, 2016

Thanks @starrify

@kmike kmike merged commit 6ff605b into scrapy:master Jun 6, 2016
2 of 3 checks passed
2 of 3 checks passed
codecov/changes 1 file has unexpected coverage changes not visible in diff.
Details
codecov/patch 100% of diff hit (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
redapple added a commit that referenced this pull request Jul 8, 2016
[backport][1.1] Retrying scrapy.core.downloader.handlers.http11.TunnelError (PR #1974)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.