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

[improve] Optimize reorderReadSequence to Check WriteSet #4478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ange-k
Copy link

@ange-k ange-k commented Aug 13, 2024

Motivation

RackawareEnsemblePlacementPolicyImpl.reorderReadSequenc has a feature to sort Bookies with poor performance.

Currently, the code checks the entire Ensemble for unavailable Bookies and performs reordering if any are found. However, since the sorting is done within the WriteSet, checking the Ensemble is overkill (except when the sizes are the same).

This change modifies the code to check and reorder only the Bookies within the WriteSet. This prevents wasteful sorting processes at the same time, future developers can understand the logic more easily without wondering if there is a specific reason for checking the entire Ensemble.

Changes

  • Modified RackawareEnsemblePlacementPolicyImpl.reorderReadSequence to check and reorder only the Bookies within the WriteSet instead of the entire Ensemble.

@ange-k ange-k changed the title Optimize reorderReadSequence to Check WriteSet [improve] Optimize reorderReadSequence to Check WriteSet Aug 16, 2024
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

Out of curiosity, how big is your "ensemble" ? I am used to values from 3 to 7, and with those numbers this patch is not a big deal

@ange-k
Copy link
Author

ange-k commented Aug 18, 2024

+1

Out of curiosity, how big is your "ensemble" ? I am used to values from 3 to 7, and with those numbers this patch is not a big deal

Thank you for your feedback. In our environment, the ensemble size is set to 6, and the WriteSet size is 2.

As you pointed out, we also recognize that the performance improvement from this code change may be minimal. However, I found the unnecessary sorting to be somewhat inefficient. While it may not have a significant impact, I felt it was worth addressing for code cleanliness, which led me to submit this PR.

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Nice catch!

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

Successfully merging this pull request may close these issues.

3 participants