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.

Sign up for GitHub

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

Jump to bottom

fix: respect forefront option in MemoryStorage's RequestQueue #2681

Merged
merged 15 commits into from
Oct 1, 2024
Merged

fix: respect forefront option in MemoryStorage's RequestQueue #2681

merged 15 commits into from
Oct 1, 2024

Conversation

barjin
Copy link
Contributor

@barjin barjin commented Sep 26, 2024 •
edited
Loading

The MemoryStorage queue implementation doesn't respect the forefront enqueue option. This PR fixes and tests this.

This PR is necessary for solving #2669, but not sufficient - the request batch-reading in RequestQueue.fetchNextRequest is the other puzzle piece.

@barjin barjin self-assigned this Sep 26, 2024
@github-actions github-actions bot added this to the 99th sprint - Tooling team milestone Sep 26, 2024
@github-actions github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Sep 26, 2024
github-actions[bot]
github-actions bot reviewed Sep 26, 2024
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Pull Request Tookit has failed!

None of the pull request and linked issue has estimate

@barjin barjin marked this pull request as draft September 26, 2024 11:55
@barjin barjin changed the title fix: respect forefront option in RQ implementations fix: respect forefront option in MemoryStorage's RequestQueue Sep 27, 2024
@barjin
Copy link
Contributor Author

barjin commented Sep 27, 2024

7efda8b removes the RequestQueue (Crawlee class) related tests - this is independent of the changes here, so I'll solve it in a separate PR.

@barjin barjin marked this pull request as ready for review September 27, 2024 10:51
janbuchar
janbuchar reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some random comments, I mostly believe that this works thanks to the tests

packages/types/src/storages.ts Outdated
@@ -204,6 +204,7 @@ export interface RequestQueueInfo {
actRunId?: string;
hadMultipleClients?: boolean;
stats?: RequestQueueStats;
forefrontRequestIds?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this one? It will only be there for local storage, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this PR is only fixing the MemoryStorage implementation. forefrontRequestIds tracks the ordering of the forefront requests (without the need to reimplement the way everything else is handled).

See requestKeyIterator (used in listHead to get the first N requests) for usage:

crawlee/packages/memory-storage/src/resource-clients/request-queue.ts

Lines 156 to 164 in 169d48b

private *requestKeyIterator(rqClient: RequestQueueClient): IterableIterator<string> {
for (let i = this.forefrontRequestIds.length - 1; i >= 0; i--) {
yield this.forefrontRequestIds[i];
}
for (const key of rqClient.requests.keys()) {
yield key;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mean... it's internal state, why do we need to expose it via getInfo()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original idea was to persist it with the other metadata, but you're right; we don't likely want to expose it.

Addressed in adce71d .

packages/memory-storage/src/resource-clients/request-queue.ts Outdated Show resolved Hide resolved
@barjin barjin merged commit b0527f9 into master Oct 1, 2024
9 checks passed
@barjin barjin deleted the fix/rq-forefront branch October 1, 2024 08:09
@barjin barjin mentioned this pull request Oct 1, 2024
Merged
barjin added a commit that referenced this pull request Oct 2, 2024
@barjin
fix: prolong- and deleteRequestLock forefront option (#2690)
cba8da3 
We neglected the `prolong` and `deleteRequestLock` methods
with #2681 , so these do not respect the `forefront`-enforced request
ordering. This PR fixes this omission.

Prerequisite for #2689 
Related to #2669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@janbuchar janbuchar janbuchar left review comments

@B4nan B4nan Awaiting requested review from B4nan

@vladfrangu vladfrangu Awaiting requested review from vladfrangu

Assignees

@barjin barjin

Labels
t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Milestone
99th sprint - Tooling team
Development

Successfully merging this pull request may close these issues.
4 participants
@barjin @janbuchar @B4nan @vladfrangu