misc(RequestQueueV2): adding notes for request queue v2 implementation #2700

drobnikj · 2024-10-09T12:55:45Z

Adding some notes for requestQueueV2 implementation.

Let's discuss.

janbuchar

Thanks @drobnikj for writing this! I briefly tested applying some of your notes on fetchNextRequest on the Python version and it seems to work so far.

I will need to read the code at least once more to make sure I get what's happening though 🙂

janbuchar · 2024-11-08T13:30:07Z

packages/core/src/storages/request_queue_v2.ts

        for (const { id, uniqueKey } of headData.items) {
            // Queue head index might be behind the main table, so ensure we don't recycle requests
            if (
+                // TODO Kuba: Missing id or uniqueKey means that the request returned from the queue is malformed. This should never happen, needs to investigate why.


I believe this is just defensive programming in a wrong place. Types support your claim that the ID and unique key should always be present. But since there is no runtime validation of the API responses (on the client side), we have no guarantee that the types actually match.

maybe we should log warning or so and do not ignore this inconsistency completely.

If the API can return objects that do not conform to the docs, maybe that should be checked on the server side? Adding code downstream for "what if the docs lie...?" is not an efficient thing to do.

janbuchar · 2024-11-08T15:47:52Z

packages/core/src/storages/request_queue_v2.ts

+                    // TODO Kuba: There is raise condition here, if you lock the request and did not unlock it, it will be locked there and run stuck.
+                    // TODO Kuba: We should at least log this.


This only happens under very specific circumstances, and even if we don't manage to unlock the request, the lock will time out in ~60 seconds, right? So we shouldn't be risking a deadlock.

60 seconds in case there is no pageTimeout set in hours which happens in google maps scraper.
I think at least warning or debug log could be there to have some idea to debug this.

Well, maybe it's a bad idea to lock requests for this long then. The RequestQueue should be able to lock for a shorter time and prolong periodically.

janbuchar · 2024-11-08T15:53:07Z

packages/core/src/storages/request_queue_v2.ts

        const headData = await this.client.listAndLockHead({
            limit: Math.min(forefront ? this.assumedForefrontCount : 25, 25),
            lockSecs: this.requestLockSecs,
        });

        const headIdBuffer = [];

+        // TODO Kuba: This is a bit of hack this should not happen. We need to investigate why.


I suppose you mean unlocking the forefront requests? I guess @barjin could provide some context here?

Anyway, this is the PR that added it https://github.com/apify/crawlee/pull/2689/files

janbuchar · 2024-11-13T10:39:03Z

packages/core/src/storages/request_provider.ts

@@ -567,6 +567,30 @@ export abstract class RequestProvider implements IStorage {
     * the function might occasionally return a false negative,
     * but it will never return a false positive.
     */
+    /**
+     * TODO Kuba:
+     * I would move this to request_queue_v2.ts file as it is related to the new API.


I agree - I will just copy the relevant bits from RequestProvider to the Request queue v2 class and remove the inheritance.

janbuchar · 2024-11-13T11:00:56Z

packages/core/src/storages/request_provider.ts

+    /**
+     * TODO Kuba:
+     * I would move this to request_queue_v2.ts file as it is related to the new API.
+     * Let's clean it up. We would probably need some new API endpoints to make this work.


I can imagine consuming the queue would be easier if we had the following endpoints:

"ensure queue head" - it would take itemCount: number, look at the items locked by the client, and if there were less than itemCount of them, it would lock more. Then it would return all requests held by that client (or IDs thereof)

"is the queue finished?" - that would contain the logic described below and probably just return true/false, maybe it could also respond "ask me later" in the case of the safety timeout

Living without 1. is pretty easy, but if we had it, it would mean less bookkeeping on Crawlee's side - we could just spam it to make sure we have stuff to work on.

2, on the other hand, would be a tremendous help.

ad. 1. This is little bit new concept, I think if we introduce this one the listAndLockHead will not make sense. Maybe we can add some parameter instead. But can you iterate over how this will help?

ad. 2. This make sense to offload to API, I just do not want to offload the whole logic, as the scraper need to decide if want to finish or not. Plus it has better idea if need to enqueue new URLs or not. Maybe just isHeadEmpty will be ok to have as API>

ad. 1. This is little bit new concept, I think if we introduce this one the listAndLockHead will not make sense. Maybe we can add some parameter instead. But can you iterate over how this will help?

I would love to make it trivial to use the API from Crawlee. If I could just call this endpoint repeatedly and it would a) return my current locked requests, b) lock more requests if possible and if I have less than itemCount and c) prolong locks on those I already have, that would be a step in that direction.

On top of that, if adding requests to the queue also returned the new "queue head", we could get rid of the whole special treatment for forefronted requests.

ad. 2. This make sense to offload to API, I just do not want to offload the whole logic, as the scraper need to decide if want to finish or not. Plus it has better idea if need to enqueue new URLs or not. Maybe just isHeadEmpty will be ok to have as API>

Which part of the logic would you like to keep in the crawler? Just because RequestQueue.isFinished() says that I can finish doesn't mean that e.g. BasicCrawler needs to obey it without question if e.g., keepAlive: true is used.

janbuchar · 2024-11-13T12:56:33Z

packages/core/src/storages/request_queue_v2.ts

        const headData = await this.client.listAndLockHead({
            limit: Math.min(forefront ? this.assumedForefrontCount : 25, 25),
            lockSecs: this.requestLockSecs,
        });

        const headIdBuffer = [];

+        // TODO Kuba: This is a bit of hack this should not happen. We need to investigate why.


Anyway, this is the PR that added it https://github.com/apify/crawlee/pull/2689/files

janbuchar · 2024-11-13T12:57:50Z

packages/core/src/storages/request_queue_v2.ts

+        // TODO Kuba: We are missing there check for the queuePausedForMigration and for aborting.
+        // If we did not pause it here, we would be fetching and locking requests.
+        // The raise condition is here that we call _clearPossibleLocks(), but after that we can still fetch and lock requests, so the requests will be locked and never unlocked.


Soo... we should just return null if queuePausedForMigration || aborting? That should prevent us from calling listAndLockHead again.

I didn't go deep in the implementation of each scrapers, but overall once a migrating or aborting event happens, we should stop working with queue and persist.

janbuchar · 2024-11-14T13:28:24Z

packages/core/src/storages/request_provider.ts

+     * - If the queueHeadIds is not empty -> happy path we have some requests to process. -> false
+     * - If the queueHeadIds is empty
+     *     -> we need to ensure that the queue is really empty(ensureHeadIsNonEmpty).
+     *     -> we need to check if other clients still processing the requests (using listHead and getRequest on the first request)


What does the getRequest accomplish here? We already established that our queue head is empty (line above), and if listHead returns a non-empty list, it means that a) some request arrived in the meantime or b) some other client is still working. Both means we are not finished, doesn't it?

If you check the line bellow, the getRequest here is only to check which type of request it is. This info we will use to give user idea about what happen and log it. e.g if there is locked request by other client we just print something like "other client still processing queue".

Pardon me, I'm stuck at "type of request" - what should I be looking for? If we're just interested in lock status, isn't it already certain that the request is locked by another client if ensureHeadIsNonEmpty returns nothing but listHead still returns requests?

Yeah, the listHead returns even locked requests plus pending. This way you can list all pending and lock requests for the queue, and then check if other clients lock these.
But as I write this I feel strange as it does not make sense 😄 . Anyway we should fix this return just pending requests without locked and introduce new API to list just lock reauests by client.

But... if I got nothing from listAndLockHead, there should be no pending requests..? Right? Or if there are (added between listAndLockHead and the subsequent listHead), isn't that what that grace period is for?

misc(RequestQueueV2): adding notes for request queue v2 implementation

2232078

github-actions bot assigned drobnikj Oct 9, 2024

github-actions bot added this to the 100th sprint - Platform Team milestone Oct 9, 2024

github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Oct 9, 2024

This comment was marked as abuse.

Sign in to view

fix: move code back

f997cff

This comment was marked as abuse.

Sign in to view

drobnikj requested review from B4nan and vladfrangu November 1, 2024 13:47

janbuchar self-requested a review November 4, 2024 11:27

janbuchar mentioned this pull request Nov 5, 2024

refactor: Simplify request queue implementation apify/crawlee-python#653

Draft

janbuchar reviewed Nov 8, 2024

View reviewed changes

janbuchar reviewed Nov 13, 2024

View reviewed changes

janbuchar reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc(RequestQueueV2): adding notes for request queue v2 implementation #2700

misc(RequestQueueV2): adding notes for request queue v2 implementation #2700

drobnikj commented Oct 9, 2024 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

janbuchar left a comment

janbuchar Nov 8, 2024

drobnikj Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 8, 2024

drobnikj Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 8, 2024

janbuchar Nov 13, 2024

janbuchar Nov 13, 2024

janbuchar Nov 13, 2024

drobnikj Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 13, 2024

janbuchar Nov 13, 2024

drobnikj Nov 19, 2024

janbuchar Nov 14, 2024

drobnikj Nov 18, 2024

janbuchar Nov 18, 2024

drobnikj Nov 19, 2024

janbuchar Nov 19, 2024

		// TODO Kuba: There is raise condition here, if you lock the request and did not unlock it, it will be locked there and run stuck.
		// TODO Kuba: We should at least log this.

misc(RequestQueueV2): adding notes for request queue v2 implementation #2700

Are you sure you want to change the base?

misc(RequestQueueV2): adding notes for request queue v2 implementation #2700

Conversation

drobnikj commented Oct 9, 2024 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

janbuchar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drobnikj commented Oct 9, 2024 •

edited

Loading