Skip to content

Conversation

@a-cid
Copy link

@a-cid a-cid commented Jun 20, 2024

Description

Modifies _set_connection_attributes to handle conn being an instance of redis.cluster.RedisCluster.

Fixes #2505

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Unit test.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I think this is missing a test to check it's working :)

@a-cid
Copy link
Author

a-cid commented Jun 21, 2024

@xrmx

I've added the requested test

Have to admit that mocking RedisCluster was a challenge.
Full disclosure, I ended up copying some code from https://github.com/redis/redis-py/blob/master/tests/test_cluster.py
Hope that's not a problem.

@a-cid a-cid requested a review from xrmx June 21, 2024 23:15
@xrmx
Copy link
Contributor

xrmx commented Jun 24, 2024

@a-cid please take a look at failing docker tests

@a-cid
Copy link
Author

a-cid commented Jun 26, 2024

@xrmx

Fixed

@a-cid a-cid requested a review from a team June 26, 2024 14:45
if hasattr(conn, "nodes_manager") and hasattr(
conn.nodes_manager.default_node, "redis_connection"
):
conn = conn.nodes_manager.default_node.redis_connection
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not access connection_kwargs directly from the NodeManager or is this a different set of kwargs?

Copy link
Author

@a-cid a-cid Sep 20, 2024

Choose a reason for hiding this comment

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

Apologies for the delay, I've been a bit busy with work.

The call to cleanup_kwargs removes host and port from the kwargs passed to the NodeManager, so we have to get them from the nodes.

@a-cid a-cid requested a review from a team as a code owner September 20, 2024 19:13
@a-cid a-cid requested a review from lzchen September 20, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

Redis instrumentation doesn't work properly with Redis Cluster

4 participants