Solr
  1. Solr
  2. SOLR-7859

Clamp down on use of System.currentTimeMillis

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.4
    • Component/s: None
    • Labels:
      None

      Description

      We did one round of this in SOLR-5734, but more places seem to keep cropping up. We should do one more round, and start whitelisting places which really need it using forbidden-apis.

      1. SOLR-7859.patch
        165 kB
        Ramkumar Aiyengar
      2. SOLR-7859.patch
        162 kB
        Ramkumar Aiyengar
      3. SOLR-7859.patch
        159 kB
        Ramkumar Aiyengar

        Issue Links

          Activity

          Hide
          Ramkumar Aiyengar added a comment - - edited

          Attached is a patch to remove most occurrences in solr code (boy there are a few!) and SuppressForbidden where the usage is legitimate.

          There are also a couple of cases where the usage is suspect, but I haven't got to it as yet. One is around stats, but the more worrying thing is that we use the wall time recorded as commit data on a commit to check if replication needs to be done. In IndexFetcher, there is this code:

                if (!forceReplication && IndexDeletionPolicyWrapper.getCommitTimestamp(commit) == latestVersion) {
                  //master and slave are already in sync just return
                  LOG.info("Slave in sync with master.");
                  successfulInstall = true;
                  return true;
                }
          

          We are checking wall times across machines to check if we are in sync? That sounds wrong.. Or I am mistaken here? Why can't we just check generations? Another place below checks both generations and timestamps to see if a full copy is needed..

                // if the generation of master is older than that of the slave , it means they are not compatible to be copied
                // then a new index directory to be created and all the files need to be copied
                boolean isFullCopyNeeded = IndexDeletionPolicyWrapper
                    .getCommitTimestamp(commit) >= latestVersion
                    || commit.getGeneration() >= latestGeneration || forceReplication;
          
          Show
          Ramkumar Aiyengar added a comment - - edited Attached is a patch to remove most occurrences in solr code (boy there are a few!) and SuppressForbidden where the usage is legitimate. There are also a couple of cases where the usage is suspect, but I haven't got to it as yet. One is around stats, but the more worrying thing is that we use the wall time recorded as commit data on a commit to check if replication needs to be done. In IndexFetcher, there is this code: if (!forceReplication && IndexDeletionPolicyWrapper.getCommitTimestamp(commit) == latestVersion) { //master and slave are already in sync just return LOG.info( "Slave in sync with master." ); successfulInstall = true ; return true ; } We are checking wall times across machines to check if we are in sync? That sounds wrong.. Or I am mistaken here? Why can't we just check generations? Another place below checks both generations and timestamps to see if a full copy is needed.. // if the generation of master is older than that of the slave , it means they are not compatible to be copied // then a new index directory to be created and all the files need to be copied boolean isFullCopyNeeded = IndexDeletionPolicyWrapper .getCommitTimestamp(commit) >= latestVersion || commit.getGeneration() >= latestGeneration || forceReplication;
          Hide
          Ramkumar Aiyengar added a comment -

          Updated patch, now fixes a few more places, removes use of core open timestamp for caching purposes in join queries in favour of nanoTime.

          Show
          Ramkumar Aiyengar added a comment - Updated patch, now fixes a few more places, removes use of core open timestamp for caching purposes in join queries in favour of nanoTime.
          Hide
          Ramkumar Aiyengar added a comment -

          Latest version, I will commit this soon before this goes out of date as well..

          Show
          Ramkumar Aiyengar added a comment - Latest version, I will commit this soon before this goes out of date as well..
          Hide
          ASF subversion and git services added a comment -

          Commit 1694798 from Ramkumar Aiyengar in branch 'dev/trunk'
          [ https://svn.apache.org/r1694798 ]

          SOLR-7859: Clamp down on use of System.currentTimeMillis

          • Use RTimer where currentTimeMillis is used for timing
          • Abstract out a new class TimeOut for when currentTimeMillis/nanoTime
            is used to timeout operations.
          • Used `new Date()` in some cases where that is the logical intent.
          • Deprecated a couple of APIs which were publicly exposing epoch time,
            in favour of Date objects to make the intent clearer.
          • A couple of cases had currentTimeMillis in dead code.
          • In some cases where currentTimeMillis was used to just generate a name,
            used nanoTime instead (really it should be some sequence/random number
            in such a case).
          • In some other cases where stamps were used for SQL queries, HTTP headers,
            comparing against data in external files, ZK etc., used SuppressForbidden.
          • Also currently allow the use of currentTimeMillis in commit data,
            this is then used in replication – this is concerning since absolute
            times are being compared, but that can be dealt with separately.
          Show
          ASF subversion and git services added a comment - Commit 1694798 from Ramkumar Aiyengar in branch 'dev/trunk' [ https://svn.apache.org/r1694798 ] SOLR-7859 : Clamp down on use of System.currentTimeMillis Use RTimer where currentTimeMillis is used for timing Abstract out a new class TimeOut for when currentTimeMillis/nanoTime is used to timeout operations. Used `new Date()` in some cases where that is the logical intent. Deprecated a couple of APIs which were publicly exposing epoch time, in favour of Date objects to make the intent clearer. A couple of cases had currentTimeMillis in dead code. In some cases where currentTimeMillis was used to just generate a name, used nanoTime instead (really it should be some sequence/random number in such a case). In some other cases where stamps were used for SQL queries, HTTP headers, comparing against data in external files, ZK etc., used SuppressForbidden. Also currently allow the use of currentTimeMillis in commit data, this is then used in replication – this is concerning since absolute times are being compared, but that can be dealt with separately.
          Hide
          Erik Hatcher added a comment -

          This commit broke bin/post, which relies on SimplePostTool. The issue is that RTimer refers to NamedList which isn't in the classpath used by the SimplePostTool startup:

          $ bin/post -c files ~/Documents
          java -classpath /Users/erikhatcher/dev/lucene-trunk/solr/dist/solr-core-6.0.0-SNAPSHOT.jar -Dauto=yes -Dc=files -Ddata=files -Drecursive=yes org.apache.solr.util.SimplePostTool /Users/erikhatcher/Documents/Lucidworks/
          SimplePostTool version 5.0.0
          Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/solr/common/util/NamedList
          	at org.apache.solr.util.SimplePostTool.execute(SimplePostTool.java:176)
          	at org.apache.solr.util.SimplePostTool.main(SimplePostTool.java:167)
          

          The best fix is probably to have bin/post also include the SolrJ library.

          And yes, there needs to be a test for bin/post - created SOLR-7901 for this task.

          Show
          Erik Hatcher added a comment - This commit broke bin/post, which relies on SimplePostTool. The issue is that RTimer refers to NamedList which isn't in the classpath used by the SimplePostTool startup: $ bin/post -c files ~/Documents java -classpath /Users/erikhatcher/dev/lucene-trunk/solr/dist/solr-core-6.0.0-SNAPSHOT.jar -Dauto=yes -Dc=files -Ddata=files -Drecursive=yes org.apache.solr.util.SimplePostTool /Users/erikhatcher/Documents/Lucidworks/ SimplePostTool version 5.0.0 Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/solr/common/util/NamedList at org.apache.solr.util.SimplePostTool.execute(SimplePostTool.java:176) at org.apache.solr.util.SimplePostTool.main(SimplePostTool.java:167) The best fix is probably to have bin/post also include the SolrJ library. And yes, there needs to be a test for bin/post - created SOLR-7901 for this task.
          Hide
          ASF subversion and git services added a comment -

          Commit 1694888 from Erik Hatcher in branch 'dev/trunk'
          [ https://svn.apache.org/r1694888 ]

          SOLR-7859: Fix bin/post to rely on SolrJ for RTimer use

          Show
          ASF subversion and git services added a comment - Commit 1694888 from Erik Hatcher in branch 'dev/trunk' [ https://svn.apache.org/r1694888 ] SOLR-7859 : Fix bin/post to rely on SolrJ for RTimer use
          Hide
          Erik Hatcher added a comment -

          Fixed bin/post on trunk. If this gets ported to 5x, don't forget the bin/post fix too.

          Show
          Erik Hatcher added a comment - Fixed bin/post on trunk. If this gets ported to 5x, don't forget the bin/post fix too.
          Hide
          Ramkumar Aiyengar added a comment -

          Thanks Erik!

          Show
          Ramkumar Aiyengar added a comment - Thanks Erik!
          Hide
          Uwe Schindler added a comment -

          But this fix still breaks post.jar if used alone. It is still often used, e.g. on windows. If you do java -jar post.jar, this fails now. We have no windows shell script at the moment.

          In my opinion, we should make post.jar working on its own and also write a test for that.

          Show
          Uwe Schindler added a comment - But this fix still breaks post.jar if used alone. It is still often used, e.g. on windows. If you do java -jar post.jar , this fails now. We have no windows shell script at the moment. In my opinion, we should make post.jar working on its own and also write a test for that.
          Hide
          Uwe Schindler added a comment -

          My personal suggestion/opinion is:

          • let SimplePostTool use System.currentTimeMillies
          • add @SuppressForbidden
            It is very important to keep post.jar working standalone.
          Show
          Uwe Schindler added a comment - My personal suggestion/opinion is: let SimplePostTool use System.currentTimeMillies add @SuppressForbidden It is very important to keep post.jar working standalone.
          Hide
          Erik Hatcher added a comment -

          Uwe Schindler oh, good points - I concur - let's switch it back to no dependencies on SimplePostTool for now.

          And maybe it's time to get Windows support... at least a very simple `bin/post.cmd` that just delegates to post.jar, allowing an "interface" to posting content that we can evolve in the future.

          Show
          Erik Hatcher added a comment - Uwe Schindler oh, good points - I concur - let's switch it back to no dependencies on SimplePostTool for now. And maybe it's time to get Windows support... at least a very simple `bin/post.cmd` that just delegates to post.jar, allowing an "interface" to posting content that we can evolve in the future.
          Hide
          Ramkumar Aiyengar added a comment -

          let SimplePostTool use System.currentTimeMillies, add @SuppressForbidden

          You can use nanoTime without RTimer anyway. But hold off in any case.. I am anyway making a change to RTimer to break the NamedList dependency, RTimer is now used in many more places, and the child timer functionality is used in only one place.

          Show
          Ramkumar Aiyengar added a comment - let SimplePostTool use System.currentTimeMillies, add @SuppressForbidden You can use nanoTime without RTimer anyway. But hold off in any case.. I am anyway making a change to RTimer to break the NamedList dependency, RTimer is now used in many more places, and the child timer functionality is used in only one place.
          Hide
          Ramkumar Aiyengar added a comment -

          Created SOLR-7902 to split out RTimer.

          Show
          Ramkumar Aiyengar added a comment - Created SOLR-7902 to split out RTimer.
          Hide
          Ramkumar Aiyengar added a comment - - edited

          Committed SOLR-7902. I haven't tried as yet, but we should be able to revert the bin/post change (without any reversion/changes to SimplePostTool, it can now use RTimer without additional deps).

          Show
          Ramkumar Aiyengar added a comment - - edited Committed SOLR-7902 . I haven't tried as yet, but we should be able to revert the bin/post change (without any reversion/changes to SimplePostTool, it can now use RTimer without additional deps).
          Hide
          Ramkumar Aiyengar added a comment -

          On second thought, I might still need to change the post.jar generation in solr/build.xml to include RTimer.class to satisfy Uwe's requirement. (If this is really a requirement, ideally SPT should move outside solr-core to a separate place where this requirement can be verified at compile time, though I find my build-system-fu lacking to do that..)

          Show
          Ramkumar Aiyengar added a comment - On second thought, I might still need to change the post.jar generation in solr/build.xml to include RTimer.class to satisfy Uwe's requirement. (If this is really a requirement, ideally SPT should move outside solr-core to a separate place where this requirement can be verified at compile time, though I find my build-system-fu lacking to do that..)
          Hide
          Uwe Schindler added a comment -

          (If this is really a requirement, ideally SPT should move outside solr-core to a separate place where this requirement can be verified at compile time, though I find my build-system-fu lacking to do that..

          +1

          Show
          Uwe Schindler added a comment - (If this is really a requirement, ideally SPT should move outside solr-core to a separate place where this requirement can be verified at compile time, though I find my build-system-fu lacking to do that.. +1
          Hide
          Erik Hatcher added a comment -

          RTimer and SimplePostTool are both in solr-core JAR at least, so java -jar dist/solr-core...jar SimplePostTool... will still do the trick.

          I think we just rollback the SPT changes per Uwe above and keep it simple. post.jar is eventually going to go away and be replaced by a bin/post(.cmd) script that can take advantage of SolrCloud, etc and will be utilizing the full capabilities of Solr's indexing facilities; we can then rely on JAR dependencies.

          Show
          Erik Hatcher added a comment - RTimer and SimplePostTool are both in solr-core JAR at least, so java -jar dist/solr-core...jar SimplePostTool... will still do the trick. I think we just rollback the SPT changes per Uwe above and keep it simple. post.jar is eventually going to go away and be replaced by a bin/post(.cmd) script that can take advantage of SolrCloud, etc and will be utilizing the full capabilities of Solr's indexing facilities; we can then rely on JAR dependencies.
          Hide
          Ramkumar Aiyengar added a comment -

          Erik Hatcher, I have already committed the RTimer refactoring, so the SolrJ dep is no longer needed. The addition of RTimer to post.jar is a one line fix, which I will soon add, so no reversion should be necessary (would be simpler than that really). I have raised SOLR-7910 for the modularization, that applies even if post.cmd is created, as we can't assume that all the dependencies of solr-core are available to use.

          Show
          Ramkumar Aiyengar added a comment - Erik Hatcher , I have already committed the RTimer refactoring, so the SolrJ dep is no longer needed. The addition of RTimer to post.jar is a one line fix, which I will soon add, so no reversion should be necessary (would be simpler than that really). I have raised SOLR-7910 for the modularization, that applies even if post.cmd is created, as we can't assume that all the dependencies of solr-core are available to use.
          Hide
          Erik Hatcher added a comment -

          +1

          If you need any help or want me to tackle it say the word. It looks like it'd be as easy as adding RTimer (and any other classes) to build.xml:

              <jar destfile="${example}/exampledocs/post.jar"
                   basedir="${dest}/solr-core/classes/java"
                   includes="org/apache/solr/util/SimplePostTool*.class">
          

          adjusting includes to add ",org/apache/solr/util/RTimer*.class"

          Show
          Erik Hatcher added a comment - +1 If you need any help or want me to tackle it say the word. It looks like it'd be as easy as adding RTimer (and any other classes) to build.xml: <jar destfile= "${example}/exampledocs/post.jar" basedir= "${dest}/solr-core/classes/java" includes= "org/apache/solr/util/SimplePostTool*.class" > adjusting includes to add ",org/apache/solr/util/RTimer*.class"
          Hide
          ASF subversion and git services added a comment -

          Commit 1695173 from Ramkumar Aiyengar in branch 'dev/trunk'
          [ https://svn.apache.org/r1695173 ]

          SOLR-7859: Add RTimer classes to post.jar

          Show
          ASF subversion and git services added a comment - Commit 1695173 from Ramkumar Aiyengar in branch 'dev/trunk' [ https://svn.apache.org/r1695173 ] SOLR-7859 : Add RTimer classes to post.jar
          Hide
          ASF subversion and git services added a comment -

          Commit 1695175 from Ramkumar Aiyengar in branch 'dev/trunk'
          [ https://svn.apache.org/r1695175 ]

          SOLR-7859: Revert r1694888 to add SolrJ to the path for bin/post, should not be needed any longer

          Show
          ASF subversion and git services added a comment - Commit 1695175 from Ramkumar Aiyengar in branch 'dev/trunk' [ https://svn.apache.org/r1695175 ] SOLR-7859 : Revert r1694888 to add SolrJ to the path for bin/post, should not be needed any longer
          Hide
          Ramkumar Aiyengar added a comment -

          Thanks for the offer Erik. This commit should hopefully address Uwe's post.jar concern. I have also now reverted your bin/post commit as it should not be needed any longer (and per my smoke test with the tool), but let me know if you see otherwise..

          Show
          Ramkumar Aiyengar added a comment - Thanks for the offer Erik. This commit should hopefully address Uwe's post.jar concern. I have also now reverted your bin/post commit as it should not be needed any longer (and per my smoke test with the tool), but let me know if you see otherwise..
          Hide
          Ramkumar Aiyengar added a comment -

          There are also a couple of cases where the usage is suspect, but I haven't got to it as yet. One is around stats, but the more worrying thing is that we use the wall time recorded as commit data on a commit to check if replication needs to be done. In IndexFetcher, there is this code:

          I have raised SOLR-7932 for this..

          Show
          Ramkumar Aiyengar added a comment - There are also a couple of cases where the usage is suspect, but I haven't got to it as yet. One is around stats, but the more worrying thing is that we use the wall time recorded as commit data on a commit to check if replication needs to be done. In IndexFetcher, there is this code: I have raised SOLR-7932 for this..
          Hide
          ASF subversion and git services added a comment -

          Commit 1696037 from Ramkumar Aiyengar in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1696037 ]

          SOLR-7859: Clamp down on use of System.currentTimeMillis

          Merges r1694798, r1695173 from trunk

          • Use RTimer where currentTimeMillis is used for timing
          • Abstract out a new class TimeOut for when currentTimeMillis/nanoTime
            is used to timeout operations.
          • Used `new Date()` in some cases where that is the logical intent.
          • Deprecated a couple of APIs which were publicly exposing epoch time,
            in favour of Date objects to make the intent clearer.
          • A couple of cases had currentTimeMillis in dead code.
          • In some cases where currentTimeMillis was used to just generate a name,
            used nanoTime instead (really it should be some sequence/random number
            in such a case).
          • In some other cases where stamps were used for SQL queries, HTTP headers,
            comparing against data in external files, ZK etc., used SuppressForbidden.
          • Also currently allow the use of currentTimeMillis in commit data,
            this is then used in replication – this is concerning since absolute
            times are being compared, but that can be dealt with separately.
          Show
          ASF subversion and git services added a comment - Commit 1696037 from Ramkumar Aiyengar in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1696037 ] SOLR-7859 : Clamp down on use of System.currentTimeMillis Merges r1694798, r1695173 from trunk Use RTimer where currentTimeMillis is used for timing Abstract out a new class TimeOut for when currentTimeMillis/nanoTime is used to timeout operations. Used `new Date()` in some cases where that is the logical intent. Deprecated a couple of APIs which were publicly exposing epoch time, in favour of Date objects to make the intent clearer. A couple of cases had currentTimeMillis in dead code. In some cases where currentTimeMillis was used to just generate a name, used nanoTime instead (really it should be some sequence/random number in such a case). In some other cases where stamps were used for SQL queries, HTTP headers, comparing against data in external files, ZK etc., used SuppressForbidden. Also currently allow the use of currentTimeMillis in commit data, this is then used in replication – this is concerning since absolute times are being compared, but that can be dealt with separately.

            People

            • Assignee:
              Ramkumar Aiyengar
              Reporter:
              Ramkumar Aiyengar
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development