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

CachingHostnameResolver #4803

Merged
merged 4 commits into from Oct 20, 2020
Merged

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented Sep 21, 2020

Fixes #4802

Based on #4802 (comment).

tests/CrawlerProcess/caching_hostname_resolver.py times out on the current master branch (git checkout master -- scrapy/resolver && tox -e py38 -- tests/test_crawler.py)

@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #4803 into master will increase coverage by 0.00%.
The diff coverage is 93.33%.

@@           Coverage Diff           @@
##           master    #4803   +/-   ##
=======================================
  Coverage   87.85%   87.86%           
=======================================
  Files         160      160           
  Lines        9735     9749   +14     
  Branches     1437     1439    +2     
=======================================
+ Hits         8553     8566   +13     
- Misses        925      926    +1     
  Partials      257      257           
Impacted Files Coverage Δ
scrapy/resolver.py 92.20% <93.33%> (+0.14%) ⬆️

@elacuesta elacuesta changed the title CachingHostnameResolver: instantiate resolution receiver CachingHostnameResolver Sep 24, 2020
@elacuesta elacuesta marked this pull request as draft September 24, 2020 18:14
@elacuesta elacuesta force-pushed the instantiate-resolution-receiver branch from d6b942c to 87feb86 Compare September 24, 2020 18:18
@elacuesta
Copy link
Member Author

(Replying to #4802 (comment))

@michael-lazar Great work, thanks. Indeed, I got confused with the return types. I observed that in practice, the return type for resolveHostName is a HostResolution object, even though the docs say it returns a IResolutionReceiver. However, storing the actual CachingResolutionReceiver object in the cache does not seem to work either 🤔

I updated this PR with the approach from the linked comment, but I'm still a bit confused. Specially, I'm not sure about importing a class from an underscored module (twisted.internet._resolver.HostResolution)

(tests are still missing!)

@Gallaecio @kmike @wRAR any thoughts?

@Gallaecio
Copy link
Member

I'm not sure about importing a class from an underscored module (twisted.internet._resolver.HostResolution)

What about just defining our own? The current upstream implementation seems rather simple.

@elacuesta
Copy link
Member Author

What about just defining our own? The current upstream implementation seems rather simple.

I like that!

@elacuesta elacuesta force-pushed the instantiate-resolution-receiver branch 2 times, most recently from ddc853b to 6a87cba Compare October 6, 2020 15:30
@elacuesta elacuesta marked this pull request as ready for review October 6, 2020 15:44
@elacuesta elacuesta closed this Oct 6, 2020
@elacuesta elacuesta reopened this Oct 6, 2020
@elacuesta elacuesta force-pushed the instantiate-resolution-receiver branch from 6a87cba to 868826b Compare October 9, 2020 13:38
@elacuesta elacuesta force-pushed the instantiate-resolution-receiver branch from 353e185 to 585e4a8 Compare October 15, 2020 12:07
wRAR
wRAR approved these changes Oct 20, 2020
@wRAR wRAR merged commit 75f35f5 into scrapy:master Oct 20, 2020
2 checks passed
@elacuesta elacuesta deleted the instantiate-resolution-receiver branch October 21, 2020 15:15
@elacuesta elacuesta restored the instantiate-resolution-receiver branch October 1, 2021 14:34
@elacuesta elacuesta deleted the instantiate-resolution-receiver branch October 1, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CachingHostnameResolver does not work with reactor.resolve()
3 participants