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

Reduce lock granularity #2514

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

Shawyeok
Copy link
Contributor

implements #2191

@yangbodong22011
Copy link
Collaborator

@Shawyeok thanks, I have a concern about "Reduce lock granularity" will bring more renewClusterSlots.

In your code, before discoverClusterSlots->w.lock();, user requests can also call getConnectionFromSlot, but the cluster route is old (renewSlotCache not yet complete), so some requests will get JedisRedirectionException and call renewSlotCache once again.

This is inconsistent with the original Jedis behavior. Do we have a way to block those requests that will return JedisRedirectionException again and let others pass?

@Shawyeok
Copy link
Contributor Author

Shawyeok commented Apr 19, 2021

In your code, before discoverClusterSlots->w.lock();, user requests can also call getConnectionFromSlot, but the cluster route is old (renewSlotCache not yet complete), so some requests will get JedisRedirectionException and call renewSlotCache once again.

@yangbodong22011 In this scenario, rediscoverLock.tryLock() will returns false immediately.

@yangbodong22011
Copy link
Collaborator

In this scenario, rediscoverLock.tryLock() will returns false immediately.

oh, nice, so the behavior can be the same as before, and the lock time of redis.clients.jedis.JedisClusterInfoCache#slots is reduced.

Copy link
Collaborator

@yangbodong22011 yangbodong22011 left a comment

Choose a reason for hiding this comment

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

LGTM

dengliming
dengliming previously approved these changes Apr 19, 2021
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

LGTM the core changes.

@sazzad16 sazzad16 added this to the 3.7.0 milestone Apr 20, 2021
Copy link
Collaborator

@dengliming dengliming left a comment

Choose a reason for hiding this comment

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

LGTM

sazzad16 pushed a commit that referenced this pull request Apr 23, 2021
@chayim chayim mentioned this pull request Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants