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

loadbalancer-experimental: support non-sequential priorities #2953

Conversation

bryce-anderson
Copy link
Contributor

Motivation:

We currently don't support priority sets that don't have sequential
groups numbers, eg we require groups 0, 1, 2, ... and will reject a
set of groups such as 0, 2, 3, ... This is an artificial constraint.

Modifications:

  • Remove the constraint and switch to a TreeMap also make us more
    resilient to an adversarial case of getting a group with a large.
  • Close an open question and fail-open if no healthy hosts are found.

@bryce-anderson bryce-anderson force-pushed the bl_anderson/support-nonconsecutive-priorities branch from 99f6626 to c20ed4d Compare June 5, 2024 15:35
Motivation:

We currently don't support priority sets that don't have sequential
groups numbers, eg we require groups 0, 1, 2, ... and will reject
a set of groups such as 0, 2, 3, ... This is an artificial constraint.

Modifications:

Lift the constraint.
@bryce-anderson bryce-anderson force-pushed the bl_anderson/support-nonconsecutive-priorities branch from c20ed4d to 49d3e77 Compare June 5, 2024 15:37
Copy link
Contributor

@tkountis tkountis left a comment

Choose a reason for hiding this comment

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

lg

// What to do if we don't have any healthy nodes at all?
if (remainingProbability > 0) {
// TODO: this is an awkward situation. Should we panic and just discard priority information?
if (weightedResults.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have ways to observe these for our observability story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add a log message for now. If you meant something like an observer then I'm not sure how that would fit nicely but we can look at it as a follow up.

@bryce-anderson bryce-anderson merged commit 26ecff5 into apple:main Jun 7, 2024
11 checks passed
@bryce-anderson bryce-anderson deleted the bl_anderson/support-nonconsecutive-priorities branch June 7, 2024 15:39
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.

3 participants