Replace Thread.sleep(..) with Future.get(timeout, :TimeUnit) in ClusterCommandExecutor.collectResults(..)
#2719
Conversation
jxblum
changed the title
Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)
jxblum
added
type: bug
jxblum
changed the title
Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)
jxblum
changed the title
Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)
jxblum
changed the title
Thread.sleep(..) call in ClusterCommandExecutor.collectResults(..)Thread.sleep(..) with Future.get(timeout, :TimeUnit) in ClusterCommandExecutor.collectResults(..)
There was a problem hiding this comment.
This pull request is a no-go and impossible to review. There are changes all over the place without the possibility to distinguish relevant changes from polishing.
It is fine to address bugs while working on code, but these musts be at least encapsulated in its own commit.
Also, a lot of these changes create the impression that they are done just because of a different code style and not an improvement over the existing code.
| } | ||
| } | ||
| return activeMasterNodes; | ||
| return getNodes().stream() |
There was a problem hiding this comment.
Stream usage introduces a huge performance penalty due to its excessive object allocation. We avoid stream usage to not accidentally introduce a performance regression unless code paths are guaranteed to run once or during the initialization phase.
There was a problem hiding this comment.
Reverted, completely.
mp911de
added
for: team-attention
|
I am not even sure how to respond to this feedback other than not all PRs can be cleanly reviewed through the GitHub lens, and sometimes should be viewed as the final product, compared to the original, in your IDE. The underlying problem from Issue #2518 boils down to this: before and after Along the way, I refactored other, redundant but related parts of the codebase to improve readability and maintainability. You cannot properly address the unnecessary and inappropriate These changes have very little to do with style vs. correctness, readability and overall long-term maintenance. I feel it is important to address issues the moment we see them and not put them off for later, or be so fine-grained it becomes an impediment to productivity, or even contributions. In summary, and in total, there were not a lot of wide sweeping changes in this PR, especially when you consider the final product. The goal is always to leave it better than I found it. You can tell me if I am wrong, but I believe I accomplished that. |
jxblum
force-pushed
the
issue/2518
branch
3 times, most recently
from
066903c to
c1890f5
Compare
Replace Thread.sleep(..) with Future.get(timeout, :TimeUnit) for 10 microseconds. As a result, Future.isDone() and Future.isCancelled() are no longer necessary. Simply try to get the results within 10 us, and if a TimeoutException is thrown, then set done to false. 10 microseconds is 1/1000 of 10 milliseconds. This means a Redis cluster with 1000 nodes will run in a similar time to Thread.sleep(10L) if all Futures are blocked waiting for the computation to complete and take an equal amount of time to compute the result, which is rarely the case in practice, given different hardware configurations, data access patterns, load balancing/request routing, and so on. However, using Future.get(timeout, :TimeUnit) is more fair than Future.get(), which blocks until a result is returned or an ExecutionException is thrown, thereby starving computationally faster nodes vs. other nodes in the cluster that might be overloaded. In the meantime, some nodes may even complete in the short amount of time when waiting on a single node to complete. 10 microseconds was partially arbitrary, but no more so than Thread.sleep(10L) (10 milliseconds). The main objective was to give each node a chance to complete the computation in a moments notice balanced with the need to quickly check if the computation is done, hence Future.get(timeout, TimeUnit.MICROSECONDS) for sub-millisecond response times. This may need to be further tuned over time, but should serve as a reasonable baseline for the time being. Additionally, this was based on https://redis.io/docs/reference/cluster-spec/#overview-of-redis-cluster-main-components in the Redis documentation, recommending a cluster size of no more than 1000 nodes. One optimization might be to reorder the Map of Futures at the end of each iteration by organizing Futures that are done first. Furthermore, Futures that have already completed could even be removed from the Map. Of course, there is little harm in keeping the completed Futures in the Map with the safeguard in place. This optimization was not included in theses changes simply because the optimization is most likely negligible and should be measured. Reconstructing a TreeMap should run mostly within log(n) time, but memory consumption should also be taken into consideration. Add test coverage for ClusterCommandExecutor collectResults(..) method. Cleanup compiler warnings in ClusterCommandExecutorUnitTests. Closes #2518
|
I reworked this PR.
This brings the change set to: |
Simplify tests. Reuse existing interfaces from Spring. Remove inappropriate nullability annotations and introduce annotations where required. Consistently name callbacks. Make exception collector concept explicit. Reformat code.
……)`. Replace Thread.sleep(..) with Future.get(timeout, :TimeUnit) for 10 microseconds. As a result, Future.isDone() and Future.isCancelled() are no longer necessary. Simply try to get the results within 10 us, and if a TimeoutException is thrown, then set done to false. 10 microseconds is 1/1000 of 10 milliseconds. This means a Redis cluster with 1000 nodes will run in a similar time to Thread.sleep(10L) if all Futures are blocked waiting for the computation to complete and take an equal amount of time to compute the result, which is rarely the case in practice, given different hardware configurations, data access patterns, load balancing/request routing, and so on. However, using Future.get(timeout, :TimeUnit) is more fair than Future.get(), which blocks until a result is returned or an ExecutionException is thrown, thereby starving computationally faster nodes vs. other nodes in the cluster that might be overloaded. In the meantime, some nodes may even complete in the short amount of time when waiting on a single node to complete. 10 microseconds was partially arbitrary, but no more so than Thread.sleep(10L) (10 milliseconds). The main objective was to give each node a chance to complete the computation in a moments notice balanced with the need to quickly check if the computation is done, hence Future.get(timeout, TimeUnit.MICROSECONDS) for sub-millisecond response times. This may need to be further tuned over time, but should serve as a reasonable baseline for the time being. Additionally, this was based on https://redis.io/docs/reference/cluster-spec/#overview-of-redis-cluster-main-components in the Redis documentation, recommending a cluster size of no more than 1000 nodes. Add test coverage for ClusterCommandExecutor collectResults(..) method. Cleanup compiler warnings in ClusterCommandExecutorUnitTests. Closes #2518 Original pull request: #2719
Simplify tests. Reuse existing interfaces from Spring. Remove inappropriate nullability annotations and introduce annotations where required. Replace Future mocking with easier to maintain and to read future method overrides. Remove superfluous code and replace with infrastructure classes provided by Spring Framework. Consistently name callbacks. Make exception collector concept explicit. Reformat code. See #2518 Original pull request: #2719
mp911de
added
type: task
|
That's merged and polished now. |
Resolves #2518
OTHER NOTABLE CHANGES
Additionally, this PR fixes a couple bugs in
collectResults(..), which were called out and captured in 2 test cases.Previously,
ClusterCommandExecutor.collectResults(..)had no test coverage.