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

Add a worker pool to the client to limit parallel requests #1107

Closed
wants to merge 6 commits into from

Conversation

DaughterOfMars
Copy link

Description of change

This PR adds a pool of workers (tasks) that accept futures, poll them, and return the result. The worker pool forms a least-recently-used cache and each worker holds a priority queue which is processed synchronously in order.

Outstanding Questions

  • Is this overkill?
    • Yes...probably.
  • Does it even solve the original problem?
    • 🤷‍♀️ hard to say

Links to any relevant issues

#1100

Type of change

Choose a type of change, and delete any options that are not relevant.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)

@Thoralf-M Thoralf-M linked an issue Sep 1, 2023 that may be closed by this pull request
self.worker_pool
.process_task(priority, async move { f(client).await })
.await
.unwrap() // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

todo 👀

types::block::{parent::Parents, payload::Payload, Block, BlockBuilder, Error as BlockError},
};

impl ClientInner {
impl Client {
Copy link
Member

Choose a reason for hiding this comment

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

Just making sure, this is not breaking in any way since Client derefs to ClientInner, right?

Copy link
Member

Choose a reason for hiding this comment

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

But wait, ClientInner is public, so this is breaking change?

Copy link
Author

Choose a reason for hiding this comment

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

Mmmm, yes I suppose so. Technically. But if you used the client this way you probably deserve to have your code break 😆

Copy link
Member

Choose a reason for hiding this comment

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

It this change (Client/ClientInner) really required for this PR? Otherwise we just do it in 2.0

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately it is most definitely required

Copy link
Author

Choose a reason for hiding this comment

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

I will think about alternative impls though. I'm not 100% with this one.

@thibault-martinez thibault-martinez deleted the feat/worker-pool branch September 13, 2023 13:37
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.

Provide option to limit requests per second
4 participants