Skip to content

fix(v2): use better seed for shard placement #3924

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

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Feb 18, 2025

The PR fixed a bad seed chosen for Fisher-Yates permutations forming the shard mapping in the new distribution algorithm.

I've noticed that in case of unavailability, the data distribution has pronounces skews and decided to test this case. I discovered that with low number of nodes and shards, the mapping we generate is very poor – the random choice is extremely unfortunate:

instance-0 [  35%][   6%] ###################################
instance-1 [  32%][  -3%] ################################
instance-2 [  32%][  -3%] ################################
-
instance-0 [   0%][ -99%]
instance-1 [  68%][ 105%] ####################################################################
instance-2 [  32%][  -3%] ################################
-
instance-0 [  35%][   6%] ###################################
instance-1 [   0%][ -99%]
instance-2 [  64%][  93%] ################################################################
-
instance-0 [  67%][ 102%] ###################################################################
instance-1 [  32%][  -3%] ################################
instance-2 [   0%][ -99%]

After some brute force (for 3-10 instances, where distribution skews are especially impactful) I found a better seed that ensures significantly better behaviour in case of node failures/unavailability:

instance-0 [  33%][   0%] #################################
instance-1 [  33%][   0%] #################################
instance-2 [  33%][   0%] #################################
-
instance-0 [   0%][ -99%]
instance-1 [  52%][  57%] ####################################################
instance-2 [  48%][  45%] ################################################
-
instance-0 [  50%][  51%] ##################################################
instance-1 [   0%][ -99%]
instance-2 [  49%][  48%] #################################################
-
instance-0 [  48%][  45%] ################################################
instance-1 [  51%][  54%] ###################################################
instance-2 [   0%][ -99%]

I'm not adding the scripts I used to find the seed, because I do not plan to maintain it: it's better to replace the randomized permutations with a knapsack solver or something similar that gives deterministic optimal results.

@kolesnikovae kolesnikovae force-pushed the fix/uniform-distribution branch from 66791a8 to 5900bb2 Compare February 18, 2025 13:07
@kolesnikovae kolesnikovae marked this pull request as ready for review February 19, 2025 01:55
@kolesnikovae kolesnikovae requested a review from a team as a code owner February 19, 2025 01:55
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM

@kolesnikovae
Copy link
Collaborator Author

Thank you Christian! This is a temporary workaround – I'm planing to revisit the approach we use to map shards to nodes

@kolesnikovae kolesnikovae merged commit c38284e into main Feb 19, 2025
21 checks passed
@kolesnikovae kolesnikovae deleted the fix/uniform-distribution branch February 19, 2025 08:20
shelldandy pushed a commit to shelldandy/pyroscope that referenced this pull request Mar 14, 2025
* fix(v2): use better seed for shard placement

* fix(v2): update tests
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