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

fix: notify autoscaled pool about newly added requests #2400

Merged
merged 2 commits into from Apr 3, 2024

Conversation

B4nan
Copy link
Member

@B4nan B4nan commented Apr 2, 2024

Notify AutoscaledPool when calling crawler.addRequests(). This is important for crawlers in keepAlive mode that might have an empty queue and we don't want to wait till the next scheduled check (which might come in as much as 500ms by default).

Related: https://apify.slack.com/archives/CD0SF6KD4/p1711660060498849

Notify AutoscaledPool when calling `crawler.addRequests()`. This is important for crawlers in `keepAlive` mode that might have an empty queue and we don't want to wait till the next scheduled check (which might come in as much as 500ms by default).

Related: https://apify.slack.com/archives/CD0SF6KD4/p1711660060498849
@github-actions github-actions bot added this to the 86th sprint - Tooling team milestone Apr 2, 2024
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Apr 2, 2024
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!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

@B4nan B4nan added the adhoc Ad-hoc unplanned task added during the sprint. label Apr 2, 2024
@B4nan B4nan requested review from vladfrangu and barjin April 2, 2024 15:06
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Maybe we should make a public method that lets you manually trigger this? LGTM otherwise

@B4nan
Copy link
Member Author

B4nan commented Apr 2, 2024

I actually had it like that initially, then I started writing the jsdoc, and I realized that the method will be called automatically from inside crawler.addRequests, so it's not really important for users. What other use cases would there be?

@vladfrangu
Copy link
Member

I guess for now there isn't any I can think off the top of my head, maybe in the future when we make DB storages too and people manually alter DBs we can provide it (if needed)

@janbuchar
Copy link
Contributor

I actually had it like that initially, then I started writing the jsdoc, and I realized that the method will be called automatically from inside crawler.addRequests, so it's not really important for users. What other use cases would there be?

If AutoscaledPool allows notifying it from the outside, it should be a part of its public API - calling a protected method from elsewhere seems like unnecessary smoke and mirrors. If anyone wants to use AutoscaledPool (I don't think many people do that), they should be able to handle the truth.

@B4nan
Copy link
Member Author

B4nan commented Apr 3, 2024

If AutoscaledPool allows notifying it from the outside, it should be a part of its public API - calling a protected method from elsewhere seems like unnecessary smoke and mirrors.

I was afraid of this academic debate... :] You could say this about every single quirk we have in the code base (and there are many, including the autoscaled pool class itself, welcome to javascript :D).

Will add the public method, as I don't feel like arguing about such a small change. But I will be very surprised if anyone ever uses this method :]

@janbuchar
Copy link
Contributor

I was afraid of this academic debate... :] You could say this about every single quirk we have in the code base (and there are many, including the autoscaled pool class itself, welcome to javascript :D).

Oh yes, yes I will.

@B4nan B4nan merged commit a90177d into master Apr 3, 2024
8 checks passed
@B4nan B4nan deleted the notify-pool-on-add-requests branch April 3, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants