You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by is...@apache.org on 2016/12/14 12:53:45 UTC
lucene-solr:jira/solr-5944: SOLR-5944: Refactoring the DUP changes to
simplify it
Repository: lucene-solr
Updated Branches:
refs/heads/jira/solr-5944 ca26b8c5c -> 2ff79c0a2
SOLR-5944: Refactoring the DUP changes to simplify it
Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/2ff79c0a
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/2ff79c0a
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/2ff79c0a
Branch: refs/heads/jira/solr-5944
Commit: 2ff79c0a277a7fcae183b52e3f2f95c44a7f7d53
Parents: ca26b8c
Author: Ishan Chattopadhyaya <ic...@gmail.com>
Authored: Wed Dec 14 19:53:10 2016 +0700
Committer: Ishan Chattopadhyaya <ic...@gmail.com>
Committed: Wed Dec 14 19:53:10 2016 +0700
----------------------------------------------------------------------
.../processor/DistributedUpdateProcessor.java | 96 ++++++--------------
1 file changed, 28 insertions(+), 68 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/2ff79c0a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
index 93c1443..24cf03b 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
@@ -1108,8 +1108,7 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
return true;
}
- if (cmd.isInPlaceUpdate()) { // nocommit: OUTER IF (see nocommits below regarding INNER ELSE)
-
+ if (cmd.isInPlaceUpdate()) {
long prev = cmd.prevVersion;
Long lastVersion = vinfo.lookupVersion(cmd.getIndexedId());
if (lastVersion == null || Math.abs(lastVersion) < prev) {
@@ -1118,57 +1117,43 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
// by the time synchronization block was entered, the prev update was deleted by DBQ. Since
// now that update is not in index, the vinfo.lookupVersion() is possibly giving us a version
// from the deleted list (which might be older than the prev update!)
- AddUpdateCommand fetchedFromLeader = fetchFullUpdateFromLeader(cmd.getPrintableId(),
- cmd.getReq().getCore().getCoreDescriptor().getCoreContainer().getUpdateShardHandler());
+ AddUpdateCommand fetchedFromLeader = fetchFullUpdateFromLeader(cmd);
- // nocommit: this log message isn't seem useful in it's current form...
- // nocommit: it doesn't explain *why* it did a "fetch from leader" let alone why anyone should care
- // nocommit: perhaps: ("In-place update of {} failed to find valid lastVersion to apply to, forced to fetch full doc from leader: {}", idBytes, ...)
- log.info("Fetched from leader: {}", (fetchedFromLeader == null? null: fetchedFromLeader.solrDoc));
+ log.info("In-place update of {} failed to find valid lastVersion to apply to, forced to fetch full doc from leader: ",
+ idBytes.utf8ToString(), (fetchedFromLeader == null? null: fetchedFromLeader.solrDoc));
if (fetchedFromLeader == null) {
log.debug("Leader told us that this document was subsequently deleted. lastVersion: " +
lastVersion + ", prev: " + prev + ", id: " + idBytes.utf8ToString() + ", lastFoundVersion: " + dependentVersionFound);
return true;
} else {
- // Newer document was fetched from the leader. Apply that document instead of this current
- // in-place update.
+ // Newer document was fetched from the leader. Apply that document instead of this current in-place update.
log.debug("Newer document is available now. lastVersion: " +
lastVersion + ", prev: " + prev + ", id: " + idBytes.utf8ToString() + ", lastFoundVersion: " + dependentVersionFound);
- // nocommit: INNER ELSE -- I'm concerned/confused by this code happening here...
- //
- // at this point, we're replacing the data in our existing "in-place" update (cmd) so it
- // becomes a normal "add" using the full SolrInputDocument fetched from the leader
- //
- // but this if/else is itself is wrapped in a bigger if (grep for "OUTER IF" above
- // and "OUTER ELSE" below) where the "else" clause does some sanity checking / processing
- // logic for "// non inplace update, i.e. full document update" -- all of which is
- // skipped for our modified "cmd"
- //
- // shouldn't the logic in that outer else clause also be applied to our
- // "no longer really an in-place" update?
+ // Make this update to become a non-inplace update containing the full document obtained from the leader
cmd.solrDoc = fetchedFromLeader.solrDoc;
cmd.prevVersion = -1;
cmd.setVersion((long)cmd.solrDoc.getFieldValue(VERSION_FIELD));
+ assert cmd.isInPlaceUpdate() == false;
}
- }
-
- if (lastVersion != null && prev < Math.abs(lastVersion)) {
- // this means we got a newer full doc update and in that case it makes no sense to apply the older
- // inplace update. Drop this update
- log.info("Update was applied on version: " + prev + ", but last version I have is: " + lastVersion
- + ". Dropping current update.");
- return true;
} else {
- // We're good, we should apply this update. First, update the bucket's highest.
- if (bucketVersion != 0 && bucketVersion < versionOnUpdate) {
- bucket.updateHighest(versionOnUpdate);
+ if (lastVersion != null && Math.abs(lastVersion) > prev) {
+ // this means we got a newer full doc update and in that case it makes no sense to apply the older
+ // inplace update. Drop this update
+ log.info("Update was applied on version: " + prev + ", but last version I have is: " + lastVersion
+ + ". Dropping current update.");
+ return true;
+ } else {
+ // We're good, we should apply this update. First, update the bucket's highest.
+ if (bucketVersion != 0 && bucketVersion < versionOnUpdate) {
+ bucket.updateHighest(versionOnUpdate);
+ }
}
}
- } else { // non inplace update, i.e. full document update
- // nocommit: OUTER ELSE (see nocommits above regarding INNER ELSE)
-
+ }
+
+ if (!cmd.isInPlaceUpdate()) {
// if we aren't the leader, then we need to check that updates were not re-ordered
if (bucketVersion != 0 && bucketVersion < versionOnUpdate) {
// we're OK... this update has a version higher than anything we've seen
@@ -1262,29 +1247,8 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
+ ". Current update should be dropped. id={}", cmd.prevVersion, lastFoundVersion, cmd.getPrintableId());
}
return -1;
- } else if (lastFoundVersion == cmd.prevVersion) {
- // nocommit: suspicious comparison: why aren't we using Math.abs(lastFoundVersion) here?
- //
- // nocommit: most conditional checks on lastFoundVersion use Math.abs(lastFoundVersion) to account
- // nocommit: for the possibility that it's negative -- which IIUC means that it was a delete
- //
- // i'm not entirely sure how/why/if the cmd.prevVersion could ever refer to a version corrisponding
- // to a delete, but it's a possibility the code should account for one way or another..
- //
- // if the answer is "should never happen" then -- as with any situation where someone tells me
- // something can never happen -- my response is "assert that it doesn't"
- //
- // nocommit: suggest replacing this "else if" clause with...
- //
- // } else if (Math.abs(lastFoundVersion) == cmd.prevVersion) {
- // assert 0 < lastFoundVersion : "prevVersion " + cmd.prevVersion + " found but is a delete!"
- // if (log.isDebugEnabled()) {
- // log.debug("Dependent update found. id={}", cmd.getPrintableId());
- // }
- // return lastFoundVersion;
- // }
- //
- // nocommit: or am i missunderstanding? is there a deliberate reason Math.abs isn't being used here?
+ } else if (Math.abs(lastFoundVersion) == cmd.prevVersion) {
+ assert 0 < lastFoundVersion : "prevVersion " + cmd.prevVersion + " found but is a delete!";
if (log.isDebugEnabled()) {
log.debug("Dependent update found. id={}", cmd.getPrintableId());
}
@@ -1295,8 +1259,7 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
log.info("Missing update, on which current in-place update depends on, hasn't arrived. id={}, looking for version={}, last found version={}",
cmd.getPrintableId(), cmd.prevVersion, lastFoundVersion);
- AddUpdateCommand missingUpdate = fetchFullUpdateFromLeader(cmd.getPrintableId(),
- cmd.getReq().getCore().getCoreDescriptor().getCoreContainer().getUpdateShardHandler());
+ AddUpdateCommand missingUpdate = fetchFullUpdateFromLeader(cmd);
if (missingUpdate == null) {
// nocommit: this message isn't helpful to users...
// nocommit: it doesn't make it clear that we know/understand why his happened and it's "ok"
@@ -1318,12 +1281,9 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
* sends a request to the shard leader to fetch the latest full document as seen on the leader.
* @return AddUpdateCommand containing latest full doc at shard leader for the given id, or null if not found.
*/
- private AddUpdateCommand fetchFullUpdateFromLeader(String id, UpdateShardHandler updateHandler) throws IOException {
- // nocommit: why does this method take in a an a String id and UpdateShardHandler
- // nocommit: when the only callers always pass the exact same values from an AddUpdateCommand?
- //
- // this method should just take in an AddUpdateCommand and let call cmd.getPrintableId() and cmd.getReq().getCore().getCoreDescriptor().getCoreContainer().getUpdateShardHandler() itself
-
+ private AddUpdateCommand fetchFullUpdateFromLeader(AddUpdateCommand inplaceAdd) throws IOException {
+ String id = inplaceAdd.getPrintableId();
+ UpdateShardHandler updateShardHandler = inplaceAdd.getReq().getCore().getCoreDescriptor().getCoreContainer().getUpdateShardHandler();
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("distrib", false);
params.set("getInputDocument", id);
@@ -1356,7 +1316,7 @@ public class DistributedUpdateProcessor extends UpdateRequestProcessor {
}
HttpSolrClient hsc = new HttpSolrClient.Builder(leaderUrl).
- withHttpClient(updateHandler.getHttpClient()).build();
+ withHttpClient(updateShardHandler.getHttpClient()).build();
NamedList rsp = null;
try {
rsp = hsc.request(ur);