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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

  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 redis-rs/glide-core matter 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 requested a review from eifrah-aws September 19, 2024 12:49
@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
@ikolomi ikolomi mentioned this pull request Sep 19, 2024
Closed
@Yury-Fridlyand
Copy link
Collaborator

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

Yury-Fridlyand
Yury-Fridlyand requested changes Sep 19, 2024
View reviewed changes
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

asafpamzn
asafpamzn reviewed Sep 22, 2024
View reviewed changes
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.

asafpamzn
asafpamzn reviewed Sep 22, 2024
View reviewed changes
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 ikolomi force-pushed the improve_retry_semantics branch from 6f07ac8 to 3d06615 Compare September 23, 2024 07:35
@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
@janhavigupta007 @ikolomi
Go:Fix benchmarking code and improve response side performance (#2288)
10fd1c8 
Signed-off-by: Janhavi Gupta <[email protected]>
@Rafiot @ikolomi
Replace google-api-python-client by protobuf (#2304)
3f0bde6 
Replace google-api-python-client with protobuf

Signed-off-by: Raphaël Vinot <[email protected]>
@ikolomi
Updated attribution files
14a37e6 
Signed-off-by: ort-bot <[email protected]>
@janhavigupta007 @ikolomi
Go: Handling generic arrays instead of only string arrays (#2289)
2f1c693 
* Go: Handling generic arrays instead of only string arrays

Signed-off-by: Janhavi Gupta <[email protected]>
@janhavigupta007 @ikolomi
Go: Adding the generic map handler and config set, get commands for s…
62da4c5 
…tandalone. (#2303)

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

Signed-off-by: Janhavi Gupta <[email protected]>
@ikolomi ikolomi force-pushed the improve_retry_semantics branch from 3d06615 to 2f64b79 Compare September 23, 2024 08:27
@ikolomi
1. Disable command retries for CME connections due to:
70a9065 
	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 <[email protected]>
@ikolomi ikolomi force-pushed the improve_retry_semantics branch from 2f64b79 to 70a9065 Compare September 23, 2024 08:30
@ikolomi ikolomi closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand requested changes

@asafpamzn asafpamzn asafpamzn approved these changes

@eifrah-aws eifrah-aws eifrah-aws approved these changes

Assignees

@ikolomi ikolomi

Labels
Core changes Used to label a PR as PR with significant changes that should trigger a full matrix tests. Rust core redis-rs/glide-core matter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ikolomi @Yury-Fridlyand @asafpamzn @eifrah-aws @janhavigupta007 @Rafiot