[
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: [email protected]
For additional commands, e-mail: [email protected]
