You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2019/12/30 22:17:30 UTC
[GitHub] [zookeeper] belugabehr opened a new pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
belugabehr opened a new pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199
…random
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] jhuan31 commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
jhuan31 commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#discussion_r363400104
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java
##########
@@ -133,7 +132,7 @@ public void run() {
// same time in the quorum
if (!stopped && deadWatchers.size() < watcherCleanThreshold) {
int maxWaitMs = (watcherCleanIntervalInSeconds
- + r.nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
+ + ThreadLocalRandom.current().nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
Review comment:
Hi @belugabehr, I wrote BlueThrottleTest and it didn't use seed. It overwrites the nextDouble() function of Random so it returns a non-random number for the test to get repeatable results. We probably can do the same for ThreadLocalRandom so it can be used?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
anmolnar commented on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#issuecomment-574106926
retest maven build
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#discussion_r362436037
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java
##########
@@ -133,7 +132,7 @@ public void run() {
// same time in the quorum
if (!stopped && deadWatchers.size() < watcherCleanThreshold) {
int maxWaitMs = (watcherCleanIntervalInSeconds
- + r.nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
+ + ThreadLocalRandom.current().nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
Review comment:
- I also found `BlueThrottle` `ZooKeeperServer` still have the old `Random`, they also needs to change to `ThreadLocalRandom.current()`?
- Please add a description about why needs this change? e.g.
`ThreadLocalRandom.current() is thread-safe and have a better performance than the old Random to avoid the Race Condition of CAS by multiple-threads
`
- You may have checkstyle violations if you found `travis-ci` failed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] jhuan31 commented on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
jhuan31 commented on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#issuecomment-571234966
Hi @belugabehr, I wrote BlueThrottleTest and it didn't use seed. It overwrites the nextDouble() function of Random so it returns a non-random number for the test to get repeatable results. We probably can do the same for ThreadLocalRandom so it can be used?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#discussion_r362436037
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java
##########
@@ -133,7 +132,7 @@ public void run() {
// same time in the quorum
if (!stopped && deadWatchers.size() < watcherCleanThreshold) {
int maxWaitMs = (watcherCleanIntervalInSeconds
- + r.nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
+ + ThreadLocalRandom.current().nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
Review comment:
- I also found `BlueThrottle` `ZooKeeperServer` still have the old `Random`, they also needs to change to `ThreadLocalRandom.current()`?
- Please add a description about why needs this change? e.g.
`ThreadLocalRandom.current() is thread-safe and have a better performance than the old Random to avoid the Race Condition of CAS by multiple-threads
`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] belugabehr edited a comment on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
belugabehr edited a comment on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#issuecomment-571252446
@jhuan31 If you need predictable Random, it is preferable to set a seed on the `Random` object. For testing, the code should be written to allow for passing in a pre-configured Random object with the seed set. One cannot extend `ThreadLocalRandom` and one cannot set a seed.
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadLocalRandom.html#setSeed-long-
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] belugabehr commented on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
belugabehr commented on issue #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#issuecomment-571252446
@jhuan31 If you need predictable Random, it is preferable to set a seed on the `Random` object. For testing, the code should be written to allow for passing in a pre-configured Random object with the seed set. One cannot extend `ThreadLocalRandom` and one set a seed.
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadLocalRandom.html#setSeed-long-
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] jhuan31 commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
jhuan31 commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#discussion_r363400104
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java
##########
@@ -133,7 +132,7 @@ public void run() {
// same time in the quorum
if (!stopped && deadWatchers.size() < watcherCleanThreshold) {
int maxWaitMs = (watcherCleanIntervalInSeconds
- + r.nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
+ + ThreadLocalRandom.current().nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
Review comment:
Hi @belugabehr, I wrote BlueThrottleTest and it didn't use seed. It overwrites the nextDouble() function of Random so it returns a non-random number for the test to get repeatable results. We probably can do the same for ThreadLocalRandom so it can be used?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] belugabehr commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1199: ZOOKEEPER-3671: Use ThreadLocalConcurrent to Replace Random and Math.…
URL: https://github.com/apache/zookeeper/pull/1199#discussion_r362567971
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatcherCleaner.java
##########
@@ -133,7 +132,7 @@ public void run() {
// same time in the quorum
if (!stopped && deadWatchers.size() < watcherCleanThreshold) {
int maxWaitMs = (watcherCleanIntervalInSeconds
- + r.nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
+ + ThreadLocalRandom.current().nextInt(watcherCleanIntervalInSeconds / 2 + 1)) * 1000;
Review comment:
Hello @maoling and thank you yet again for the review.
I will address checkstyle violations.
In regards to the places that I left, they take advantage of the ability to set a `seed` for Random for testing purposes. I didn't want to change that capability.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services