Skip to content

Support retries in MultiClusterWrapper#737

Open
gbanasiak wants to merge 4 commits intoelastic:masterfrom
gbanasiak:retryable-multi-cluster-wrapper
Open

Support retries in MultiClusterWrapper#737
gbanasiak wants to merge 4 commits intoelastic:masterfrom
gbanasiak:retryable-multi-cluster-wrapper

Conversation

@gbanasiak
Copy link
Contributor

@gbanasiak gbanasiak commented Jan 30, 2025

The multi-cluster-wrapper custom runner unwraps the runner before calling it for every remote cluster. This strips the wrapper responsible for retries (src) from the runner making operation parameters such as retry-until-success ignored.

Another wrapper removed during the unwrap is MultiClientRunner (src, src). This wrapper assumes that ES client is specified through a {"<cluster-name>": <client>} dictionary, and for single-cluster runners (all built-in Rally runners) this wrapper extracts the client from default cluster, while for multi-cluster runners (custom ones), provides entire dictionary.

This PR tries to address it by resigning from the unwrap. For this to work ES client is passed in a dictionary with default key which assumes the underlying runner will be a single-cluster one.

@gbanasiak gbanasiak requested a review from a team January 30, 2025 20:37
@gbanasiak
Copy link
Contributor Author

I couldn't make unit tests work locally for me without Python and package upgrades.

prereq:
pyenv install --skip-existing $(PY38)
pyenv local $(PY38)
pyenv install --skip-existing $(PY312)
Copy link
Member

Choose a reason for hiding this comment

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

How come we jump to 3.12 instead of 3.9 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This follows dev environment preparation from Rally's Makefile, where 3.12 is used as well. It aligns both.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I hadnt spotted we had switched there (We used to use 3.8, then when we upgraded to 3.12 we jumped straight to 3.12). I suppose from a testing perspective we still test on 3.9, so thats ok I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, we test 3.9-3.12, but for dev purposes configure 3.12.

Comment on lines 220 to 221
# call original runner assuming a single-cluster one, see https://github.com/elastic/rally/pull/488
# and https://github.com/elastic/rally/pull/1563 for additional context
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we cannot assume that, this requires a modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've generalized in e3f42ea. Today multi-cluster-wrapper is only used with built-in Rally runners which are exclusively single-cluster. But I can imagine a situation where someone wants to iterate through all the clusters providing a dictionary of clusters for the runner to choose from. Perhaps that's too much.

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