[jira] [Commented] (ZOOKEEPER-1990) suspicious instantiation of java Random instances

["Alexander Shraer (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-1990?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14075231#comment-14075231 ] 

Alexander Shraer commented on ZOOKEEPER-1990:
---------------------------------------------

Regarding StaticHostProvider.java - Randomness stuff was introduced in ZK-1355 to make sure that each client has a different permutation of the servers, and that client-side rebalancing after reconfig ends up with equal load.

75:       sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());

If I remember correctly the goal here is to make sure that when multiple client threads start at the same time on the same machine (such as in our systest) and execute this line each one is likely to have a different random seed. 

src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java
40:    private Random r = new Random(1);
src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
98:        sourceOfRandomness = new Random(randomnessSeed);

These two are only used in tests. I want repeatable results, hence the seed of 1. But I still want each client to have a StaticHostProvider that creates a unique permutation, so I pass the StaticHostProvider a randomness seed like this:

    private StaticHostProvider getHostProvider(byte size) {
        return new StaticHostProvider(getServerAddresses(size), r.nextLong());
    }


> suspicious instantiation of java Random instances
> -------------------------------------------------
>
>                 Key: ZOOKEEPER-1990
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1990
>             Project: ZooKeeper
>          Issue Type: Bug
>    Affects Versions: 3.5.0
>            Reporter: Patrick Hunt
>            Priority: Critical
>             Fix For: 3.5.0
>
>
> It's not clear to me why we are doing this, but it looks very suspicious. Why aren't we just calling "new Random()" in these cases? (even for the tests I don't really see it - typically that would just be for repeatability)
> {noformat}
> ag "new Random[ \t]*\(" .
> src/java/main/org/apache/zookeeper/ClientCnxn.java
> 817:        private Random r = new Random(System.nanoTime());        
> src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
> 75:       sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
> 98:        sourceOfRandomness = new Random(randomnessSeed);
> src/java/main/org/apache/zookeeper/server/quorum/AuthFastLeaderElection.java
> 420:                rand = new Random(java.lang.Thread.currentThread().getId()
> src/java/main/org/apache/zookeeper/server/SyncRequestProcessor.java
> 64:    private final Random r = new Random(System.nanoTime());
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
> 537:        Random r = new Random(id ^ superSecret);
> 554:        Random r = new Random(sessionId ^ superSecret);
> src/java/test/org/apache/zookeeper/server/quorum/WatchLeakTest.java
> 271:        Random r = new Random(SESSION_ID ^ superSecret);
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
> 151:            Random rand = new Random(Thread.currentThread().getId());
> 258:            Random rand = new Random(Thread.currentThread().getId());
> 288:        Random rand = new Random(Thread.currentThread().getId());
> src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java
> 40:    private Random r = new Random(1);
> {noformat}



--
This message was sent by Atlassian JIRA
(v6.2#6252)