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 retry semantics #2330

Closed
wants to merge 6 commits into from
Closed

Improve retry semantics #2330

wants to merge 6 commits into from

Conversation

ikolomi
Copy link
Collaborator

@ikolomi ikolomi commented Sep 19, 2024

  1. Disable command retries for CME connections due to:
    a. Absence of command retry configuration in client creation params making retry mechanics unexpected, confusing and
    inconsistent with command timeout constrains.
    b. Retry semantics are inconsistent between the CME and CMD client since CMD lacks the retry logic

Issue: #2251

@ikolomi ikolomi added Rust core Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. labels Sep 19, 2024
@ikolomi ikolomi self-assigned this Sep 19, 2024
@ikolomi ikolomi requested a review from a team as a code owner September 19, 2024 12:49
@Yury-Fridlyand
Copy link
Collaborator

You're missing DCO.
Please list PRs and/or changes you're integrating from the submodule.

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please add changelog

Copy link
Collaborator

@asafpamzn asafpamzn left a comment

Choose a reason for hiding this comment

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

It depends on which scenarios we retry, if the connection is not established we can retry until there is a timeout. We do you want to remove it.

Copy link
Collaborator

@asafpamzn asafpamzn left a comment

Choose a reason for hiding this comment

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

Which error the user will get?
Is it timeout out or connection failure?

@ikolomi
Copy link
Collaborator Author

ikolomi commented Sep 23, 2024

@Yury-Fridlyand change log added
@asafpamzn discussed

@ikolomi ikolomi changed the base branch from main to release-1.1 September 23, 2024 08:17
janhavigupta007 and others added 5 commits September 23, 2024 11:23
Signed-off-by: Janhavi Gupta <janhavigupta@google.com>
Replace google-api-python-client with protobuf

Signed-off-by: Raphaël Vinot <raphael@vinot.info>
Signed-off-by: ort-bot <valkey-glide@lists.valkey.io>
* Go: Handling generic arrays instead of only string arrays

Signed-off-by: Janhavi Gupta <janhavigupta@google.com>
…tandalone. (#2303)

* Go:Implement generic map handler and config get,set for standalone.

Signed-off-by: Janhavi Gupta <janhavigupta@google.com>
	a. Absence of command retry configuration in client creation params making retry mechanics unexpected, confusing and inconsistent with command timeout constrains.
	b. Retry semantics are inconsistent between the CME and CMD client since CMD lacks the retry logic

Signed-off-by: ikolomi <ikolomin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. Rust core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants