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

Add track parameters to control target throughput and clients for search operations #1

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

Conversation

engechas
Copy link

@engechas engechas commented Nov 29, 2021

Background
This change adds target_throughput and search_clients as track parameters for all search operations where a default target throughput is specified. If no user-specified value for these new parameters exists, then the existing defaults will be used. Otherwise, the user-specified value will be honored. There is a special case for target-throughput where 'none' as a string can be specified to remove the target-throughput setting.

This change will allow our runs to be more configurable and is a step in the direction of automated saturation testing.

Additionally, it will allow us to remove target throughput on our daily benchmarks to get more significant query performance numbers.

Testing
I spun up the beta AMI and switched the tracks remote to my repo and pulled all branches. The I tested the following:

  1. Ran all challenges for all tracks for all branches (except master) in test mode with no track-params specified.

The only failures were in eventdata (which this PR doesn't touch) when running the transform challenge, and the expected failures of 'challenge does not exist' or 'track does not exist' for lower distribution versions.

  1. Repeated 1 with search_clients:2,target_throughput:'none' as track-params

I did not run eventdata for step 2 as it's unchanged by this PR. There were no new failures except for SO, which threw an error saying the params were unsupported, which is the expected behavior.

@engechas engechas marked this pull request as ready for review December 1, 2021 17:52
Copy link

@IanHoang IanHoang left a comment

Choose a reason for hiding this comment

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

LGTM

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