lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Shalin Shekhar Mangar (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SOLR-5944) Support updates of numeric DocValues
Date Tue, 19 Jul 2016 20:04:20 GMT

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

Shalin Shekhar Mangar commented on SOLR-5944:
---------------------------------------------

Thanks Ishan. This is great progress since the last time I reviewed this patch.

I've only skimmed the latest patch but in particular I find a few problems in DistributedUpdateProcessor#waitForDependentUpdates:
# This method doesn't look correct. All places which call vinfo.lookupVersion() do it after
acquiring a read lock by calling vinfo.lockForUpdate(). Looking at the code for vinfo.lookupVersion()
it accesses the map and prevMap in updateLog which can be modified by a writer thread while
waitForDependentUpdates is reading their values. So first of all we need to ensure that we
acquire and release the read lock. Acquiring this lock and then waiting on a different object
(the "bucket") will not cause a deadlock condition because it is a read lock (which can be
held by multiple threads).
# Secondly, this method can be made more efficient. It currently wakes up every 100ms and
reads the new "lastFoundVersion" from the update log or index. This is wasteful. A better
way would be to wait for the timeout period directly before calling {{vinfo.lookupVersion()}}
inside the synchronized block.
# Similar to #1 -- calling {{vinfo.lookupVersion()}} after {{fetchMissingUpdateFromLeader}}
should be done after acquiring a read lock.
# There is no reason to synchronize on bucket when calling the {{versionAdd}} method again
because it will acquire the monitor anyway.
# DistributedUpdateProcessor#waitForDependentUpdates uses wrong javadoc tag '@returns' instead
of '@return'
# The debug log message should be moved out of the loop instead of introducing a debugMessagePrinted
boolean flag
# Use the org.apache.solr.util.TimeOut class for timed wait loops
# Method can be made private

I've attempted to write a better wait-loop here (warning: not tested):
{code}
long prev = cmd.prevVersion;
    long lastFoundVersion = 0;


    TimeOut timeOut = new TimeOut(5, TimeUnit.SECONDS);
    vinfo.lockForUpdate();
    try {
      synchronized (bucket) {
        lastFoundVersion = vinfo.lookupVersion(cmd.getIndexedId());
        while (lastFoundVersion < prev && !timeOut.hasTimedOut())  {
          if (log.isDebugEnabled()) {
            log.debug("Re-ordered inplace update. version=" + (cmd.getVersion() == 0 ? versionOnUpdate
: cmd.getVersion()) +
                ", prevVersion=" + prev + ", lastVersion=" + lastFoundVersion + ", replayOrPeerSync="
+ isReplayOrPeersync);
          }
          try {
            bucket.wait(5000);
          } catch (InterruptedException ie) {
            throw new RuntimeException(ie);
          }
          lastFoundVersion = vinfo.lookupVersion(cmd.getIndexedId());
        }
      }
    } finally {
      vinfo.unlockForUpdate();
    }

// check lastFoundVersion against prev again and handle all conditions
{code}

However I think that since the read lock and bucket monitor has to be acquired by this method
anyway, it might be a good idea to just call it from inside versionAdd after acquiring those
monitors. Then this method can focus on just waiting for dependent updates and nothing else.

A random comment on the changes made to DebugFilter: The setDelay mechanism introduced here
may be a good candidate for Mark's new TestInjection#injectUpdateRandomPause?

> Support updates of numeric DocValues
> ------------------------------------
>
>                 Key: SOLR-5944
>                 URL: https://issues.apache.org/jira/browse/SOLR-5944
>             Project: Solr
>          Issue Type: New Feature
>            Reporter: Ishan Chattopadhyaya
>            Assignee: Shalin Shekhar Mangar
>         Attachments: DUP.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
SOLR-5944.patch, SOLR-5944.patch, TestStressInPlaceUpdates.eb044ac71.beast-167-failure.stdout.txt,
TestStressInPlaceUpdates.eb044ac71.beast-587-failure.stdout.txt, TestStressInPlaceUpdates.eb044ac71.failures.tar.gz,
hoss.62D328FA1DEA57FD.fail.txt, hoss.62D328FA1DEA57FD.fail2.txt, hoss.62D328FA1DEA57FD.fail3.txt,
hoss.D768DD9443A98DC.fail.txt, hoss.D768DD9443A98DC.pass.txt
>
>
> LUCENE-5189 introduced support for updates to numeric docvalues. It would be really nice
to have Solr support this.



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


Mime
View raw message