Issue #1248: Avoid queueing multiple cluster discovers of the same type #1249
Conversation
| @@ -22,6 +22,7 @@ | |||
| private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(); | |||
| private final Lock r = rwl.readLock(); | |||
| private final Lock w = rwl.writeLock(); | |||
| private volatile int currentRediscoverProcessType; | |||
There was a problem hiding this comment.
Can we make this a private ENUM so we can better name the discovering type?
|
@Spikhalskiy does it make sense to have two discovering types? Because |
|
@marcosnils I just think that code with flag only is very error prone. Somebody could add new discovering on any other case (or make discoverClusterNodesAndSlots not only on startup time), copy-paste logic with only flag and doesn't think that this logic became broken (discover task of one type cancel task of another type). So no, it's doesn't make much sense now, but it could make sense if future - I hope somebody will take a look at different types and think why they have been added. But I don't have strong opinion, if you think that it should be fine as boolean flag - I could rework it. |
|
@Spikhalskiy I understand your opinion and I agree with it to some point. I don't like to add code "just in case something happens in the future" as it may never happen and you have stuff that might confuse someone how's reading the code. I prefer to keep thing simple and use a descriptive boolean flag as |
|
@marcosnils got it, addressed. |
|
@Spikhalskiy LGTM. Once another contributor reviews we can mearge. |
|
LGTM. Nice finding @Spikhalskiy and thanks for crafting patch. |
|
Merged into branches master, 2.8, 2.9 respectively! |
|
@HeartSaVioR @marcosnils Great, thanks for review. |
| //If rediscovering is already in process - no need to start one more same rediscovering, just return | ||
| if (!rediscovering) { | ||
| w.lock(); | ||
| rediscovering = true; |
There was a problem hiding this comment.
Late to the party, but a possible perf enhancement here might be to check rediscovering again:
if (!rediscovering) {
w.lock();
if (rediscovering) {
return;
}
That way, if multiple threads make it past the initial check when under high load and block at lock acquisition, only the first one will do the work.
There was a problem hiding this comment.
It shouldn't help. We set it to true only after lock and to false before releasing lock, so if thread got write lock - it's always false in this var.
There was a problem hiding this comment.
Ah, right. This looks like a candidate for a double-checked locking pattern, but because we're resetting rediscovering it won't work. Darn. It'd be nice to be able to get rid of those extra rediscovers.
(And this, friends, is why I shouldn't read PRs before coffee.)
Address issue with queueing multiple JedisCluster state rediscovering from Issue #1248