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

Simpify assignment of workload parameters target_throughout and search_clients #82

Conversation

tlfeng
Copy link
Contributor

@tlfeng tlfeng commented May 23, 2023

Description

Simplify the assignment of parameters target-throughput and search_clients to reduce the code duplication, which was introduced in the commit dd2ff3b.
Initially I planned to use Jinja macro syntax to reuse the redundant codes, but after looking at the target codes, I think simplifying the code itself might be a better option.

For target_throughput, I propose to change from

{%- if not target_throughput %}
,"target-throughput": 2
{%- elif target_throughput is string and target_throughput.lower() == 'none' %}
{%- else %}
,"target-throughput": {{ target_throughput | tojson }}
{%- endif %}

to

"target-throughput": {{ target_throughput | default(2) }}

For search_clients, I propose to change from

{%- if search_clients is defined and search_clients %}
,"clients": {{ search_clients | tojson}}
{%- endif %}

to

{%- if search_clients is defined %}
"clients": {{ search_clients }}
{%- endif %}

or from

{%- if not search_clients %}
,"clients": 2
{%- elif search_clients is defined and search_clients %}
,"clients": {{ search_clients | tojson}}
{%- endif %}

to

"clients": {{ search_clients | default(2) }}

Some reasons:

  1. For the field target-throughput, there was a specified value assigned to everywhere has got the field, so it's typical to use "target-throughput": {{ target_throughput | default(2) }} to assign a default value if adding the ability to assign custom value. This way is commonly used in the repository, such as https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/1/nyc_taxis/test_procedures/default.json#L34 and https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/1/so/test_procedures/default.json#L12, and there is no need to use the current tedious statements for "target-throughput".
  2. Validating the data type of the input value is not useful, and there is no such validation for other parameters in this repository.
    Although there is a difference in the error message after the code change, but I think it's acceptable, because there is no such tedious value validation for other fields.
    For example, assigning non-number value to target-throughput after my code change will throw the following error.
opensearch-benchmark execute-test \
--workload-path=/home/ftianli/github/opensearch-benchmark-workloads/nyc_taxis --test-procedure append-no-conflicts --test-mode \
--workload-params="target_throughput:text,search_clients:4"

[ERROR] Cannot execute-test. Error in test execution orchestrator (Could not load '/home/ftianli/github/opensearch-benchmark-workloads/nyc_taxis/workload.json': Expecting value: line 454 column 32 (char 11316). Lines containing the error:

          "operation": "default",
          "warmup-iterations": 50,
          "iterations": 100,
          "target-throughput": text
-------------------------------^ Error is here
          ,"clients": 4
        },

The below is the error before my code change:
I guess the error message comes from opensearch-benchmark validates the user input of "target-throughput", although the field is not assigned in json file.

[ERROR] Cannot execute-test. Error in test execution orchestrator (Task [default] specifies invalid target throughput [text].)
  1. Looks like the expression and search_clients in existing statement if search_clients is defined and search_clients make no sense in this case. Empty value such as None or none or "" still be assigned in json file.
  2. I don't think the expression | tojson is necessary here in ,"clients": {{ search_clients | tojson}}, looking at the document https://jinja.palletsprojects.com/en/2.11.x/templates/#tojson, tojson() used to escape certain special characters in HTML contexts. The 2 parameters target_throughout and search_clients has number value, and I don't think escaping character is necessary here.
    Besides, using expression | tojson seems comes from Elasticsearch code elastic/rally-tracks@dcd691f, but don't understand the meaning of that.

Testing:

opensearch-benchmark execute-test \
--workload-path=/home/ftianli/github/opensearch-benchmark-workloads/nyc_taxis --test-procedure append-no-conflicts --test-mode \
--workload-params="target_throughput:10,search_clients:4"
opensearch-benchmark execute-test \
--workload-path=/home/ftianli/github/opensearch-benchmark-workloads/noaa --test-procedure append-no-conflicts --test-mode \
--workload-params="target_throughput:10,search_clients:4"

Issues Resolved

#64

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@tlfeng tlfeng force-pushed the target_throughput_search_clients_simplify branch 2 times, most recently from 3a8139b to b3c6a5a Compare May 23, 2023 23:10
…h_clients

Signed-off-by: Tianli Feng <ftianli@amazon.com>
@tlfeng tlfeng force-pushed the target_throughput_search_clients_simplify branch from b3c6a5a to 315b023 Compare May 23, 2023 23:27
@tlfeng
Copy link
Contributor Author

tlfeng commented Jun 8, 2023

I will close the PR for now, although there is no obvious issue with the code change. I will discuss the necessity with the maintainers for this repository.

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.

1 participant