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

Rework queue and errors in prover proxy #947

Closed
SantiagoPittella opened this issue Nov 1, 2024 · 6 comments
Closed

Rework queue and errors in prover proxy #947

SantiagoPittella opened this issue Nov 1, 2024 · 6 comments
Assignees
Milestone

Comments

@SantiagoPittella
Copy link
Collaborator

What should be done?

We can refactor a couple methods of the HttpProxy implementation in the prover proxy, simplifying the request ID handling and queues.

How should it be done?

  • We can replace the sleep in upstream_peer with Notify.
  • Simplify queueing and dequeueing.
  • Use requestID from request context instead of headers.
  • Move dequeuing in case of connection error into fail_to_connect method.

When is this task done?

The task is done when requestID isn't a header anymore and worker's queues are simplified.

Additional context

No response

@SantiagoPittella
Copy link
Collaborator Author

We may consider adding initial configs in the init command for the tx-prover CLI in this issue too:
#955 (comment)

@SantiagoPittella SantiagoPittella self-assigned this Nov 15, 2024
@SantiagoPittella
Copy link
Collaborator Author

As talked with @igamigo , I'm going to implement a single queue to be shared between all workers.
Using Tokio::Notify, when a worker is available to take a job, it will send a notification, and the first request of the queue should be picked up by the worker.

@bobbinth
Copy link
Contributor

As talked with @igamigo , I'm going to implement a single queue to be shared between all workers.
Using Tokio::Notify, when a worker is available to take a job, it will send a notification, and the first request of the queue should be picked up by the worker.

This could work, but also, would the approach similar to the one we have now not work? Basically, keep the status of the worker in the internal data structure and update it when the worker completes the task. Or does this not work for some reason?

@SantiagoPittella
Copy link
Collaborator Author

That works, though in that case, when all workers are occupied, we need to keep the timer that we have in the upstream_peer method to look in this data structure if a worker is available.
With Notify we add a more complexity but get rid of the timer.

Will go for the internal structure first though, and then we can decide if we want to pivot to Notify.

@SantiagoPittella
Copy link
Collaborator Author

May we consider this as done?

Cc @bobbinth

@bobbinth
Copy link
Contributor

Yes - definitely!

Closed by #976.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants