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

Default ClientOptions.timeoutOptions to TimeoutOptions.enabled() #2927

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

thachlp
Copy link
Contributor

@thachlp thachlp commented Jul 22, 2024

Issue #2518
Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@tishun
Copy link
Collaborator

tishun commented Aug 7, 2024

Hey @thachlp , there is a number of tests that fail after this change, most likely because they do not expect these timeout options to be set by default. Could you address this?

@tishun
Copy link
Collaborator

tishun commented Aug 8, 2024

Hey @thachlp , there is a number of tests that fail after this change, most likely because they do not expect these timeout options to be set by default. Could you address this?

Also make sure you merge the fix from #2948

@thachlp
Copy link
Contributor Author

thachlp commented Aug 8, 2024

Hey @thachlp , there is a number of tests that fail after this change, most likely because they do not expect these timeout options to be set by default. Could you address this?

Let me check

@tishun tishun added the type: feature A new feature label Aug 8, 2024
@tishun tishun added this to the 6.5.0.RELEASE milestone Aug 8, 2024
@thachlp
Copy link
Contributor Author

thachlp commented Aug 9, 2024

Hey @thachlp , there is a number of tests that fail after this change, most likely because they do not expect these timeout options to be set by default. Could you address this?

@tishun I updated the test, please help review it.

Copy link
Collaborator

@tishun tishun left a comment

Choose a reason for hiding this comment

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

Looks good to me

Right now this fix is targeted for 7.x, I'll discuss this in the team and see if we should merge it to a 7.x branch or change the target version. I am personally leaning for changing the target branch.

@thachlp
Copy link
Contributor Author

thachlp commented Aug 16, 2024

Looks good to me

Right now this fix is targeted for 7.x, I'll discuss this in the team and see if we should merge it to a 7.x branch or change the target version. I am personally leaning for changing the target branch.

Thanks @tishun

@tishun tishun merged commit cf215de into redis:main Sep 13, 2024
5 checks passed
@thachlp thachlp deleted the update-default-timeout-options branch September 13, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants