Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The current code is good, it avoids SecureRandom/blocking when we don't need real security (just used for safety checks).

      On the other hand it has some downsides:

      • the sources of randomness here aren't the best, e.g. sysprops will be the same when using automated deployment tools if the jvm is the same version, installed in the same place, same user, etc.
      • asking for a Properties of all the sysprops needs blanket read-write access to all of them, which is inconvenient if you want to lock this down in tests (which I do). Today this means you can't ban write access or lucene won't work.

      I think we should use /dev/urandom when its available, its just practical and exactly what we need. If its not available (e.g. windows) we can use the current logic. If sysprops arent available we can just use another hashcode instead and lucene can still be used.

        Activity

        Hide
        Robert Muir added a comment -

        here is a patch.

        Show
        Robert Muir added a comment - here is a patch.
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Hoss Man added a comment -

        i never realized this "psuedo-randomness" id generating code existed until today ... it definitely seems like it could easily contributed to hard to reproduce test failures between multiple machines – but if i'm reading this right, at least prior to this patch the same JVM, with the same command line args should produce a consistent sequence of ids .... correct?

        But with this patch even that level of "same machine, same JVM args" reproducibility goes away doesn't it?

        would be be worthwhile to make this static init code try, as a first attempt, to read the "tests.seed" system property, and if it's set then use a hashcode of that string to seed everything else (superceeding /dev/urandom, the sysprops, nanoTime, etc...)

        Wouldn't that give us best of all worlds: more (reproducible) randomness in JVMs running tests, and more randomness in non-test applications.

        Show
        Hoss Man added a comment - i never realized this "psuedo-randomness" id generating code existed until today ... it definitely seems like it could easily contributed to hard to reproduce test failures between multiple machines – but if i'm reading this right, at least prior to this patch the same JVM, with the same command line args should produce a consistent sequence of ids .... correct? But with this patch even that level of "same machine, same JVM args" reproducibility goes away doesn't it? would be be worthwhile to make this static init code try, as a first attempt, to read the "tests.seed" system property, and if it's set then use a hashcode of that string to seed everything else (superceeding /dev/urandom, the sysprops, nanoTime, etc...) Wouldn't that give us best of all worlds: more (reproducible) randomness in JVMs running tests, and more randomness in non-test applications.
        Hide
        Robert Muir added a comment -

        would be be worthwhile to make this static init code try, as a first attempt, to read the "tests.seed" system property, and if it's set then use a hashcode of that string to seed everything else (superceeding /dev/urandom, the sysprops, nanoTime, etc...)

        We already do this, this is not changed by the patch. That is why all this logic is in an 'else' block.

        Show
        Robert Muir added a comment - would be be worthwhile to make this static init code try, as a first attempt, to read the "tests.seed" system property, and if it's set then use a hashcode of that string to seed everything else (superceeding /dev/urandom, the sysprops, nanoTime, etc...) We already do this, this is not changed by the patch. That is why all this logic is in an 'else' block.
        Hide
        Uwe Schindler added a comment -

        Strong +1

        I was always not happy with the sysprops!

        Show
        Uwe Schindler added a comment - Strong +1 I was always not happy with the sysprops!
        Hide
        Hoss Man added a comment -

        We already do this, this is not changed by the patch. That is why all this logic is in an 'else' block.

        Bah ... sorry. i saw "GOOD_FAST_HASH_SEED" already set by the sysprop, but didn't see it get used in the code you were modifiying ... didn't notice the second call to System.getProperty("tests.seed") there.

        +1

        Show
        Hoss Man added a comment - We already do this, this is not changed by the patch. That is why all this logic is in an 'else' block. Bah ... sorry. i saw "GOOD_FAST_HASH_SEED" already set by the sysprop, but didn't see it get used in the code you were modifiying ... didn't notice the second call to System.getProperty("tests.seed") there. +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1662141 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1662141 ]

        LUCENE-6292: seed StringHelper better

        Show
        ASF subversion and git services added a comment - Commit 1662141 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1662141 ] LUCENE-6292 : seed StringHelper better
        Hide
        ASF subversion and git services added a comment -

        Commit 1662147 from Robert Muir in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1662147 ]

        LUCENE-6292: seed StringHelper better

        Show
        ASF subversion and git services added a comment - Commit 1662147 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662147 ] LUCENE-6292 : seed StringHelper better
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development