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

feat: support Elasticache cluster mode by setting IsClusterMode in config #3255

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

reeshijoshi
Copy link
Contributor

  • This is required because Elasticache supports setting up cluster mode with configuration endpoint. So the check len(Addrs) > 1 is not enough for determining cluster mode.
  • If IsClusterMode is true, the client will be created as a cluster client.

@ndyakov
Copy link
Collaborator

ndyakov commented Jan 29, 2025

@reeshijoshi thank you for the contribution. Would you be able to add tests for this new option?

@reeshijoshi
Copy link
Contributor Author

@reeshijoshi thank you for the contribution. Would you be able to add tests for this new option?

This needs an Elasticache configuration endpoint to be provided in the Addrs field. I can add a unit test with one address only in this but that's not representative of how this would behave in the actual scenario

@reeshijoshi
Copy link
Contributor Author

@reeshijoshi thank you for the contribution. Would you be able to add tests for this new option?

Added a test that asserts client type

@ndyakov
Copy link
Collaborator

ndyakov commented Jan 30, 2025

@vladvildanov I think this one is good to be merged.

@ndyakov ndyakov self-requested a review January 30, 2025 15:29
Copy link
Collaborator

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

@reeshijoshi Overall looks good to me, would you mind changing the comment as suggested or anything starting with the IsClusterMode as suggested in https://tip.golang.org/doc/comment

ndyakov
ndyakov previously approved these changes Jan 31, 2025
Copy link
Collaborator

@ndyakov ndyakov 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, thanks @reeshijoshi

@ndyakov
Copy link
Collaborator

ndyakov commented Feb 20, 2025

@reeshijoshi would you please add additional test so we can merge this?

@reeshijoshi
Copy link
Contributor Author

@reeshijoshi would you please add additional test so we can merge this?

done

@ndyakov ndyakov merged commit 7d55118 into redis:master Feb 26, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants