Bulk action refresh param

[inqueue]

This commit adds support for the bulk ?refresh query parameter to address feedback for a new workload for serverless. Possible values for refresh are true (?refresh=true) for async, false (?refresh=false), and wait_for (?refresh=wait_for) for sync, consistent with https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh.

[b-deam]

Approach looks fine to me, but curious how you intend to handle more complex configurations where want to change the refresh param dynamically? Will we require users to implement a custom param source?

[inqueue]

Approach looks fine to me, but curious how you intend to handle more complex configurations where want to change the refresh param dynamically? Will we require users to implement a custom param source?

I am not too concerned with changing the refresh param dynamically. I do not have a solution for doing this in a deterministic way. For now, it is good enough to issue a manual refresh to the target in a parallel operation. Right now, I am more concerned with the use of the refresh parameter to avoid situations where search-idle optimizations kick in for smaller corpora where the use case of this parameter is most applicable.

Does it make more sense to change the possible values to strings only? Like:

• sync - Equivalent to ?refresh=wait_for
• async - Equivalent to ?refesh=true
• default - Equivalent to ?refresh=false

This would leave the door open for a future parameter value like sometimes where ?refresh=true (or ?refresh=wait_for) for a subset of requests as you have suggested elsewhere.

[inqueue]

Does it make more sense to change the possible values to strings only? Like:

• sync - Equivalent to ?refresh=wait_for
• async - Equivalent to ?refesh=true
• default - Equivalent to ?refresh=false

The latest commit uses this model. It can be tested against serverless with the track in elastic/rally-tracks#425.

esrally race --track=serverless_k8s --pipeline=benchmark-only --client-options='{"default": {"use_ssl": true, "verify_certs": false, "basic_auth_user":"${ESUSER}","basic_auth_password":"${ESPASSWORD}"}}' --telemetry=node-stats,shard-stats,data-stream-stats --telemetry-params="node-stats:node-stats-include-indices" --target-hosts=${TARGET} --track-params="bulk_size:100,ingest_percentage:10,bulk_refresh:async" --kill-running-processes --test-mode

[b-deam]

Left some comments, but it otherwise LGTM.

[b-deam]

Sorry for my last comment, I see now what you're checking for. I think both INFO and WARNING aren't enough this case, and we should probably just error out because of an invalid value - otherwise we risk returning misleading results and the corresponding log message is buried deep in the file somewhere.

Maybe we can error out similar to how it's done here?:

rally/esrally/driver/runner.py

Lines 2609 to 2612 in b1a822c

	if op_type not in self.supported_op_types:
	raise exceptions.RallyAssertionError(
	f"Unsupported operation-type [{op_type}]. Use one of [{', '.join(self.supported_op_types)}]."
	)

[b-deam]

We could probably also just lean on the client itself to return an error, if you pass something in that's not valid does it return anything meaningful?

[b-deam]

Left one minor comment around unsupported values, but no need for another review cycle. LGTM.