You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ho...@apache.org on 2017/01/13 04:46:42 UTC
[1/2] lucene-solr:jira/solr-5944: removed some missguided nocommits
and added a comment clarifying why the types of assertions i thought could be
added actually can't
Repository: lucene-solr
Updated Branches:
refs/heads/jira/solr-5944 619d1283c -> bb5dfb619
removed some missguided nocommits and added a comment clarifying why the types of assertions i thought could be added actually can't
Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/938fe58b
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/938fe58b
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/938fe58b
Branch: refs/heads/jira/solr-5944
Commit: 938fe58b5ef7b3cdfb6c1fa19981b3283d156525
Parents: 619d128
Author: Chris Hostetter <ho...@apache.org>
Authored: Thu Jan 12 15:38:56 2017 -0700
Committer: Chris Hostetter <ho...@apache.org>
Committed: Thu Jan 12 15:38:56 2017 -0700
----------------------------------------------------------------------
.../solr/cloud/TestStressInPlaceUpdates.java | 35 ++++++++------------
1 file changed, 14 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/938fe58b/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java b/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
index 8f758c9..497e53a 100644
--- a/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
+++ b/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
@@ -233,14 +233,6 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
DocInfo currInfo = model.get(id);
if (null != returnedVersion &&
(Math.abs(returnedVersion.longValue()) > Math.abs(currInfo.version))) {
-
- // nocommit: CHANGE TO THIS...
- //
- // Preserve the values from the old model in the deleted doc.
- // This way the values will always be increasing if/when the doc is re-added, and the
- // read threads can always assert that if a doc is found with a newer version then expected
- // then the field values must be greater then the ones expected.
-
model.put(id, new DocInfo(returnedVersion.longValue(), 0, 0));
}
}
@@ -396,8 +388,20 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
assertEquals(msg + " ...intVal and longVal in solr doc are internally (modulo) inconsistent w/eachother",
0, (longVal % intVal));
- // nocommit: for expected.version < foundVersion, assert expected.values < doc.values
- // nocommit: (see previous nocommit in writer thread)
+ // NOTE: when foundVersion is greater then the version read from the model,
+ // it's not possible to make any assertions about the field values in solr relative to the
+ // field values in the model -- ie: we can *NOT* assert expected.longFieldVal <= doc.longVal
+ //
+ // it's tempting to think that this would be possible if we changed our model to preserve the
+ // "old" valuess when doing a delete, but that's still no garuntee because of how oportunistic
+ // concurrency works with negative versions: When adding a doc, we can assert that it must not
+ // exist with version<0, but we can't assert that the *reason* it doesn't exist was because of
+ // a delete with the specific version of "-42".
+ // So a wrtier thread might (1) prep to add a doc for the first time with "intValue=1,_version_=-1",
+ // and that add may succeed and (2) return some version X which is put in the model. but
+ // inbetween #1 and #2 other threads may have added & deleted the doc repeatedly, updating
+ // the model with intValue=7,_version_=-42, and a reader thread might meanwhile read from the
+ // model before #2 and expect intValue=5, but get intValue=1 from solr (with a greater version)
} else {
fail(String.format(Locale.ENGLISH, "There were more than one result: {}", response));
@@ -436,13 +440,8 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
// sanity check of the model agrees...
assertTrue(msg + " is deleted/non-existent in Solr, but model has non-neg version",
expected.version < 0);
- // nocommit: (see related nocommit in writer thread about updating model for deletes)
- // nocommit: if deleted deleted docs preserve old model and never have 0,0 values, change asserts...
assertEquals(msg + " is deleted/non-existent in Solr", expected.intFieldValue, 0);
assertEquals(msg + " is deleted/non-existent in Solr", expected.longFieldValue, 0);
- // nocommit: ...to this...
- // assertEquals(msg + " is deleted/non-existent in Solr, but model has inconsistent (modulo) values",
- // 0, (expected.longFieldValue % expected.intFieldValue));
} else {
msg = msg + " <==VS==> " + actual;
assertEquals(msg, expected.intFieldValue, actual.getFieldValue("val1_i_dvo"));
@@ -469,16 +468,10 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
DocInfo info = committedModel.get(i);
if (info.version < 0) {
// first, a quick sanity check of the model itself...
-
- // nocommit: (see related nocommit in writer thread about updating model for deletes)
- // nocommit: if deleted deleted docs preserve old model and never have 0,0 values, change asserts...
assertEquals("Inconsistent int value in model for deleted doc" + i + "=" + info,
0, info.intFieldValue);
assertEquals("Inconsistent long value in model for deleted doc" + i + "=" + info,
0L, info.longFieldValue);
- // nocommit: ...to this...
- // assertEquals("Inconsistent (modulo) values in model for deleted doc" + i + "=" + info,
- // 0, (info.longFieldValue % info.intFieldValue));
committedModel.remove(i);
}
[2/2] lucene-solr:jira/solr-5944: SOLR-5944: fix incorrect log
messages,
and add comment clarifying why a final check of the model can only happen
after a final commit
Posted by ho...@apache.org.
SOLR-5944: fix incorrect log messages, and add comment clarifying why a final check of the model can only happen after a final commit
Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/bb5dfb61
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/bb5dfb61
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/bb5dfb61
Branch: refs/heads/jira/solr-5944
Commit: bb5dfb619470a3648cdd2a97e80fb6c1c829aaa6
Parents: 938fe58
Author: Chris Hostetter <ho...@apache.org>
Authored: Thu Jan 12 17:37:26 2017 -0700
Committer: Chris Hostetter <ho...@apache.org>
Committed: Thu Jan 12 17:37:26 2017 -0700
----------------------------------------------------------------------
.../solr/cloud/TestStressInPlaceUpdates.java | 45 ++++++++++++--------
1 file changed, 28 insertions(+), 17 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/bb5dfb61/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java b/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
index 497e53a..0debeb4 100644
--- a/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
+++ b/solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java
@@ -1,4 +1,3 @@
-
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
@@ -210,22 +209,25 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
if (rand.nextBoolean()) Thread.yield();
if (oper < commitPercent + deletePercent + deleteByQueryPercent) {
- log.info("deleting id {}: {}",id,info);
+ final boolean dbq = (oper >= commitPercent + deletePercent);
+ final String delType = dbq ? "DBI": "DBQ";
+ log.info("{} id {}: {}", delType, id, info);
Long returnedVersion = null;
try {
- final boolean dbq = (oper >= commitPercent + deletePercent);
returnedVersion = deleteDocAndGetVersion(Integer.toString(id), params("_version_", Long.toString(info.version)), dbq);
- log.info((dbq? "DBI": "DBQ")+": Deleting id=" + id + "], version=" + info.version
- + ". Returned version=" + returnedVersion);
+ log.info(delType + ": Deleting id=" + id + ", version=" + info.version
+ + ". Returned version=" + returnedVersion);
} catch (RuntimeException e) {
if (e.getMessage() != null && e.getMessage().contains("version conflict")
|| e.getMessage() != null && e.getMessage().contains("Conflict")) {
// Its okay for a leader to reject a concurrent request
- log.warn("Conflict during partial update, rejected id=" + id + ", " + e);
+ log.warn("Conflict during {}, rejected id={}, {}", delType, id, e);
returnedVersion = null;
- } else throw e;
+ } else {
+ throw e;
+ }
}
// only update model if update had no conflict & the version is newer
@@ -257,9 +259,11 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
if (e.getMessage() != null && e.getMessage().contains("version conflict")
|| e.getMessage() != null && e.getMessage().contains("Conflict")) {
// Its okay for a leader to reject a concurrent request
- log.warn("Conflict during partial update, rejected id=" + id + ", " + e);
+ log.warn("Conflict during full update, rejected id={}, {}", id, e);
returnedVersion = null;
- } else throw e;
+ } else {
+ throw e;
+ }
}
} else {
// PARTIAL
@@ -271,11 +275,13 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
if (e.getMessage() != null && e.getMessage().contains("version conflict")
|| e.getMessage() != null && e.getMessage().contains("Conflict")) {
// Its okay for a leader to reject a concurrent request
- log.warn("Conflict during full update, rejected id=" + id + ", " + e);
+ log.warn("Conflict during partial update, rejected id={}, {}", id, e);
} else if (e.getMessage() != null && e.getMessage().contains("Document not found for update.")
- && e.getMessage().contains("id="+id)) {
- log.warn("Attempting a partial update for a recently deleted document, rejected id=" + id + ", " + e);
- } else throw e;
+ && e.getMessage().contains("id="+id)) {
+ log.warn("Attempted a partial update for a recently deleted document, rejected id={}, {}", id, e);
+ } else {
+ throw e;
+ }
returnedVersion = null;
}
}
@@ -455,12 +461,17 @@ public class TestStressInPlaceUpdates extends AbstractFullDistribZkTestBase {
}
{ // do a final search and compare every result with the model
+
+ // because commits don't provide any sort of concrete versioning (or optimistic concurrency constraints)
+ // there's no way to garuntee that our committedModel matches what was in Solr at the time of the last commit.
+ // It's possible other threads made additional writes to solr before the commit was processed, but after
+ // the committedModel variable was assigned it's new value.
+ //
+ // what we can do however, is commit all completed updates, and *then* compare solr search results
+ // against the (new) committed model....
- // nocommit: START: better way to check the original committedModel????
- // nocommit: do we really need to wait for shards to catchup, or is this just a red herring for another bug?
- waitForThingsToLevelOut(30); // NOTE: this does an automatic commit...
+ waitForThingsToLevelOut(30); // NOTE: this does an automatic commit for us & ensures replicas are up to date
committedModel = new HashMap<>(model);
- // nocommit: END
// first, prune the model of any docs that have negative versions
// ie: were never actually added, or were ultimately deleted.