fix(scrapers): Use RequestQueue directly to avoid excessive RQ writes on runs with large startUrls list

[janbuchar]

• affects all generic scrapers
• closes Increased RQ write usage in generic scrapers #392
• the change means that we will skip the RQ+RL tandem behavior from BasicCrawler
• an alternative solution would be to initialize the tandem lazily, but this would be tricky due to 1) the amount of non-trivial code that depends on requestQueue being present and 2) input containing the requestQueueName option that we'd also need to take into account

Caveats

• with large startUrls, there will be a lot of RQ writes billed at the start of the Actor
  • this should not be a huge deal
  • however, the same bunch of writes will happen on a resume/migration/whatever - maybe we should add a useState flag so that we can only insert the startUrls once

[B4nan]

do we event want to go through the RL?

[janbuchar]

We only use it here to parse the input conveniently. We could pass the input directly to addRequests, but we couldn't pick only N first requests because the startUrls can contain links to files that contain multiple links.

[B4nan]

when i mentioned the overenqueuing for 150% of the limit, i was mainly concerned about the requests added via enqueueLinks, i don't think its common that people provide more start URLs than the limit itself, the issue is what we enqueue on those pages. i guess we'd have to handle this on crawlee level.

[janbuchar]

i don't think its common that people provide more start URLs than the limit itself

Bold claim. I can imagine for example

1. someone setting a limit to get a quick sample of the results when developing their pageFunction
2. parsing URLs from an external source that's out of control of the user - it would make sense to add a limit in that case to prevent mayhem

The 150% is there because the crawl may fail for some URLs.

[B4nan]

i am not saying we shouldnt handle this here, i am saying the important part is elsewhere

[barjin]

+1 for processing this in Crawlee, see apify/crawlee#2728.

Also, this.input.maxResultsPerCrawl is slightly confusing naming here - even if pageFunction returns null (or failedRequestHandler is called), it generates an "empty" dataset record (see #353).

IIRC maxResultsPerCrawl here = maxRequestsPerCrawl in Crawlee.

[barjin]

Let's see how it goes 🚀

[barjin]

I'm thinking this (initializing all of the Request instances preemptively) could potentially cause some OOM issues (especially with Cheerio crawler, which users tend to run with smaller RAM allocation). We are basically reverting this PR.

Either way, there are no breaking changes in the API. Let's release it - reverting is always an option 🤷

[janbuchar]

You raise a good point here.

[janbuchar]

If we do this - apify/crawlee#2980 - we can make this thingy more efficient.

[barjin]

however, the same bunch of writes will happen on a resume/migration/whatever - maybe we should add a useState flag so that we can only insert the startUrls once

Oh yes, we might want to take care of this. I recall similar logic in WCC. Not having this raised the RQ write count significantly for some longer runs with migrations and resurrects.

[janbuchar]

however, the same bunch of writes will happen on a resume/migration/whatever - maybe we should add a useState flag so that we can only insert the startUrls once

Oh yes, we might want to take care of this. I recall similar logic in WCC. Not having this raised the RQ write count significantly for some longer runs with migrations and resurrects.

Yeah, it surely is an eyesore 😁 Do you think it makes sense to handle this flag in Crawlee?

[barjin]

handle this flag in Crawlee

Yes, preferably we could handle this inside of crawler.run([...list of requests...]) here, because looking at the code, we have the exact same issue there. But still, I'd support only this native interface - if any user (including us) decides to transfer requests from one storage to another before running the crawler like we do here, that's imho on them (us).

[barjin]

The state maintenance looks valid 👍

I can think of a situation with requestQueueName input that will behave differently after these changes (two runs, shared RQ, not shared KVS), but it's IMO such an edge-case I wouldn't even consider this breaking.