[jira] [Comment Edited] (SOLR-7859) Clamp down on use of System.currentTimeMillis

["Ramkumar Aiyengar (JIRA)" <ji...@apache.org>]

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

Ramkumar Aiyengar edited comment on SOLR-7859 at 8/4/15 2:38 AM:
-----------------------------------------------------------------

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:

{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;
      }
{code}

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..

{code}
      // 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;
{code}


was (Author: andyetitmoves):
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:

{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;
      }
{code}

We are checking wall times across machines to check if we are in sync, that sounds wrong. Or I am mistaken here?

> Clamp down on use of System.currentTimeMillis
> ---------------------------------------------
>
>                 Key: SOLR-7859
>                 URL: https://issues.apache.org/jira/browse/SOLR-7859
>             Project: Solr
>          Issue Type: Improvement
>            Reporter: Ramkumar Aiyengar
>         Attachments: SOLR-7859.patch
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org