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

feat: Custom Balancer #590

Open
wants to merge 89 commits into
base: current
Choose a base branch
from
Open

feat: Custom Balancer #590

wants to merge 89 commits into from

Conversation

metcoder95
Copy link
Member

@metcoder95 metcoder95 commented Jun 12, 2024

metcoder95 and others added 30 commits September 13, 2023 12:59
… 6.12.0 (#453)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@metcoder95 metcoder95 changed the base branch from next to current October 2, 2024 08:28

export type PiscinaTaskBalancer = (
task: PiscinaTask,
workers: PiscinaWorker[]
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doubtful about wether or not pass the workers on every call or make the balancer rely directly on the events.

By passing them on every call, balancer will always have the up-to-date list of workers, but might cause them to need to constantly adapt and lack possibly the state.

On the event based approach they get definitely more control; maybe keep both?

@metcoder95 metcoder95 marked this pull request as ready for review October 2, 2024 09:18
@metcoder95
Copy link
Member Author

@ronag @jerome-benoit this should be ready for a review for landing; so far the balancer has been decoupled from an scheduler.

I'll update docs and more later this week

@jerome-benoit
Copy link

@ronag @jerome-benoit this should be ready for a review for landing; so far the balancer has been decoupled from an scheduler.

Great!

I do not think I will be able to do a review before next week ...

@metcoder95
Copy link
Member Author

No rush! I'll proceed with the documentation this week

src/index.ts Outdated Show resolved Hide resolved
this._addNewWorker();
}

if (task.abortSignal) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (task.abortSignal) {
if (task.abortSignal?.aborted) {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do that check already in upper layers, if it reaches here, it means the task is not aborted yet.

Copy link
Collaborator

@ronag ronag left a comment

Choose a reason for hiding this comment

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

RSLGTM. A bit too much for me to get a good overview. Would be great with some docs and samples. I think sorting out the public API is higher prio than the implementation details.

@metcoder95
Copy link
Member Author

SGTM, I'll start preparing either Tomorrow or later in the weekend

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

Successfully merging this pull request may close these issues.

8 participants