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(spreadscaler): Assign leftover instances by weight, instead of spread order #173

Merged

Conversation

ahmedtadde
Copy link
Contributor

@ahmedtadde ahmedtadde commented Sep 12, 2023

Feature or Problem

The existing implementation of compute_spread favors spread requirements defined earlier in the YAML, rather than the ones with the highest weight. With the updated implementation, leftover replicas are distributed based on weight.

Related Issues

Addresses #154

Release Information

v0.6.0

Consumer Impact

N/A

Testing

Built on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Tested on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Unit Test(s)

The following tests are modified to align with the new implementation where order by weight does matter.

Acceptance or Integration

Manual Verification

…licas based on weight

Signed-off-by: Ahmed Tadde <ahmedtaddde@gmail.com>
Signed-off-by: Ahmed Tadde <ahmedtadde@gmail.com>
@ahmedtadde ahmedtadde force-pushed the feat/compute-spread-enhancement branch from ea91193 to 765806f Compare September 12, 2023 09:33
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

This is an elegant fix! I have one requested change around re-sorting the computed spreads, leaning towards not needing to do it at all.

src/scaler/spreadscaler/mod.rs Outdated Show resolved Hide resolved
…l for computed spreads output

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
src/scaler/spreadscaler/mod.rs Outdated Show resolved Hide resolved
src/scaler/spreadscaler/mod.rs Outdated Show resolved Hide resolved
…hould be fine; will be revisited later if needed.

Signed-off-by: Ahmed <ahmedtadde@gmail.com>
@brooksmtownsend
Copy link
Member

Thank you @ahmedtadde 🎉

@brooksmtownsend brooksmtownsend merged commit 942a811 into wasmCloud:main Sep 14, 2023
5 checks passed
@ahmedtadde ahmedtadde deleted the feat/compute-spread-enhancement branch September 14, 2023 17:12
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.

2 participants