You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by sa...@apache.org on 2014/07/08 05:20:55 UTC
svn commit: r1608646 - in /lucene/dev/trunk/solr: ./
core/src/java/org/apache/solr/rest/schema/
core/src/java/org/apache/solr/schema/
core/src/java/org/apache/solr/update/processor/
core/src/test/org/apache/solr/schema/
Author: sarowe
Date: Tue Jul 8 03:20:54 2014
New Revision: 1608646
URL: http://svn.apache.org/r1608646
Log:
SOLR-6180: Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock.
Modified:
lucene/dev/trunk/solr/CHANGES.txt
lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/CopyFieldCollectionResource.java
lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldCollectionResource.java
lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldResource.java
lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/IndexSchema.java
lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java
lucene/dev/trunk/solr/core/src/java/org/apache/solr/update/processor/AddSchemaFieldsUpdateProcessorFactory.java
lucene/dev/trunk/solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchemaConcurrent.java
Modified: lucene/dev/trunk/solr/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/CHANGES.txt?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/CHANGES.txt (original)
+++ lucene/dev/trunk/solr/CHANGES.txt Tue Jul 8 03:20:54 2014
@@ -157,6 +157,9 @@ Bug Fixes
* SOLR-6223: SearchComponents may throw NPE when using shards.tolerant and there is a failure
in the 'GET_FIELDS/GET_HIGHLIGHTS/GET_DEBUG' phase. (Tomás Fernández Löbbe via shalin)
+
+* SOLR-6180: Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock.
+ (Gregory Chanan via Steve Rowe)
Optimizations
---------------------
Modified: lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/CopyFieldCollectionResource.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/CopyFieldCollectionResource.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/CopyFieldCollectionResource.java (original)
+++ lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/CopyFieldCollectionResource.java Tue Jul 8 03:20:54 2014
@@ -171,12 +171,14 @@ public class CopyFieldCollectionResource
boolean success = false;
while (!success) {
try {
- IndexSchema newSchema = oldSchema.addCopyFields(fieldsToCopy);
- if (null != newSchema) {
- getSolrCore().setLatestSchema(newSchema);
- success = true;
- } else {
- throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add fields.");
+ synchronized (oldSchema.getSchemaUpdateLock()) {
+ IndexSchema newSchema = oldSchema.addCopyFields(fieldsToCopy);
+ if (null != newSchema) {
+ getSolrCore().setLatestSchema(newSchema);
+ success = true;
+ } else {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add fields.");
+ }
}
} catch (ManagedIndexSchema.SchemaChangedInZkException e) {
log.debug("Schema changed while processing request, retrying");
Modified: lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldCollectionResource.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldCollectionResource.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldCollectionResource.java (original)
+++ lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldCollectionResource.java Tue Jul 8 03:20:54 2014
@@ -195,12 +195,14 @@ public class FieldCollectionResource ext
}
}
firstAttempt = false;
- IndexSchema newSchema = oldSchema.addFields(newFields, copyFields);
- if (null != newSchema) {
- getSolrCore().setLatestSchema(newSchema);
- success = true;
- } else {
- throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add fields.");
+ synchronized (oldSchema.getSchemaUpdateLock()) {
+ IndexSchema newSchema = oldSchema.addFields(newFields, copyFields);
+ if (null != newSchema) {
+ getSolrCore().setLatestSchema(newSchema);
+ success = true;
+ } else {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add fields.");
+ }
}
} catch (ManagedIndexSchema.SchemaChangedInZkException e) {
log.debug("Schema changed while processing request, retrying");
Modified: lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldResource.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldResource.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldResource.java (original)
+++ lucene/dev/trunk/solr/core/src/java/org/apache/solr/rest/schema/FieldResource.java Tue Jul 8 03:20:54 2014
@@ -166,12 +166,14 @@ public class FieldResource extends BaseF
while (!success) {
try {
SchemaField newField = oldSchema.newField(fieldName, fieldType, map);
- IndexSchema newSchema = oldSchema.addField(newField, copyFieldNames);
- if (null != newSchema) {
- getSolrCore().setLatestSchema(newSchema);
- success = true;
- } else {
- throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add field.");
+ synchronized (oldSchema.getSchemaUpdateLock()) {
+ IndexSchema newSchema = oldSchema.addField(newField, copyFieldNames);
+ if (null != newSchema) {
+ getSolrCore().setLatestSchema(newSchema);
+ success = true;
+ } else {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add field.");
+ }
}
} catch (ManagedIndexSchema.SchemaChangedInZkException e) {
log.debug("Schema changed while processing request, retrying");
Modified: lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/IndexSchema.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/IndexSchema.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/IndexSchema.java (original)
+++ lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/IndexSchema.java Tue Jul 8 03:20:54 2014
@@ -1473,7 +1473,9 @@ public class IndexSchema {
}
/**
- * Copies this schema, adds the given field to the copy, then persists the new schema.
+ * Copies this schema, adds the given field to the copy, then persists the
+ * new schema. Requires synchronizing on the object returned by
+ * {@link #getSchemaUpdateLock()}.
*
* @param newField the SchemaField to add
* @return a new IndexSchema based on this schema with newField added
@@ -1486,7 +1488,9 @@ public class IndexSchema {
}
/**
- * Copies this schema, adds the given field to the copy, then persists the new schema.
+ * Copies this schema, adds the given field to the copy, then persists the
+ * new schema. Requires synchronizing on the object returned by
+ * {@link #getSchemaUpdateLock()}.
*
* @param newField the SchemaField to add
* @param copyFieldNames 0 or more names of targets to copy this field to. The targets must already exist.
@@ -1500,7 +1504,9 @@ public class IndexSchema {
}
/**
- * Copies this schema, adds the given fields to the copy, then persists the new schema.
+ * Copies this schema, adds the given fields to the copy, then persists the
+ * new schema. Requires synchronizing on the object returned by
+ * {@link #getSchemaUpdateLock()}.
*
* @param newFields the SchemaFields to add
* @return a new IndexSchema based on this schema with newFields added
@@ -1513,7 +1519,9 @@ public class IndexSchema {
}
/**
- * Copies this schema, adds the given fields to the copy, then persists the new schema.
+ * Copies this schema, adds the given fields to the copy, then persists the
+ * new schema. Requires synchronizing on the object returned by
+ * {@link #getSchemaUpdateLock()}.
*
* @param newFields the SchemaFields to add
* @param copyFieldNames 0 or more names of targets to copy this field to. The target fields must already exist.
@@ -1527,7 +1535,10 @@ public class IndexSchema {
}
/**
- * Copies this schema and adds the new copy fields to the copy, then persists the new schema
+ * Copies this schema and adds the new copy fields to the copy, then
+ * persists the new schema. Requires synchronizing on the object returned by
+ * {@link #getSchemaUpdateLock()}.
+ *
* @param copyFields Key is the name of the source field name, value is a collection of target field names. Fields must exist.
* @return The new Schema with the copy fields added
*/
@@ -1554,4 +1565,16 @@ public class IndexSchema {
log.error(msg);
throw new SolrException(ErrorCode.SERVER_ERROR, msg);
}
+
+ /**
+ * Returns the schema update lock that should be synchronzied on
+ * to update the schema. Only applicable to mutable schemas.
+ *
+ * @return the schema update lock object to synchronize on
+ */
+ public Object getSchemaUpdateLock() {
+ String msg = "This IndexSchema is not mutable.";
+ log.error(msg);
+ throw new SolrException(ErrorCode.SERVER_ERROR, msg);
+ }
}
Modified: lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java (original)
+++ lucene/dev/trunk/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java Tue Jul 8 03:20:54 2014
@@ -209,33 +209,30 @@ public final class ManagedIndexSchema ex
if (copyFieldNames == null){
copyFieldNames = Collections.emptyMap();
}
- // even though fields is volatile, we need to synchronize to avoid two addFields
- // happening concurrently (and ending up missing one of them)
- synchronized (getSchemaUpdateLock()) {
- newSchema = shallowCopy(true);
-
- for (SchemaField newField : newFields) {
- if (null != newSchema.getFieldOrNull(newField.getName())) {
- String msg = "Field '" + newField.getName() + "' already exists.";
- throw new FieldExistsException(ErrorCode.BAD_REQUEST, msg);
- }
- newSchema.fields.put(newField.getName(), newField);
+ newSchema = shallowCopy(true);
- if (null != newField.getDefaultValue()) {
- log.debug(newField.getName() + " contains default value: " + newField.getDefaultValue());
- newSchema.fieldsWithDefaultValue.add(newField);
- }
- if (newField.isRequired()) {
- log.debug("{} is required in this schema", newField.getName());
- newSchema.requiredFields.add(newField);
- }
- Collection<String> copyFields = copyFieldNames.get(newField.getName());
- if (copyFields != null) {
- for (String copyField : copyFields) {
- newSchema.registerCopyField(newField.getName(), copyField);
- }
+ for (SchemaField newField : newFields) {
+ if (null != newSchema.getFieldOrNull(newField.getName())) {
+ String msg = "Field '" + newField.getName() + "' already exists.";
+ throw new FieldExistsException(ErrorCode.BAD_REQUEST, msg);
+ }
+ newSchema.fields.put(newField.getName(), newField);
+
+ if (null != newField.getDefaultValue()) {
+ log.debug(newField.getName() + " contains default value: " + newField.getDefaultValue());
+ newSchema.fieldsWithDefaultValue.add(newField);
+ }
+ if (newField.isRequired()) {
+ log.debug("{} is required in this schema", newField.getName());
+ newSchema.requiredFields.add(newField);
+ }
+ Collection<String> copyFields = copyFieldNames.get(newField.getName());
+ if (copyFields != null) {
+ for (String copyField : copyFields) {
+ newSchema.registerCopyField(newField.getName(), copyField);
}
}
+
// Run the callbacks on SchemaAware now that everything else is done
for (SchemaAware aware : newSchema.schemaAware) {
aware.inform(newSchema);
@@ -261,30 +258,26 @@ public final class ManagedIndexSchema ex
ManagedIndexSchema newSchema = null;
if (isMutable) {
boolean success = false;
- // even though fields is volatile, we need to synchronize to avoid two addCopyFields
- // happening concurrently (and ending up missing one of them)
- synchronized (getSchemaUpdateLock()) {
- newSchema = shallowCopy(true);
- for (Map.Entry<String, Collection<String>> entry : copyFields.entrySet()) {
- //Key is the name of the field, values are the destinations
+ newSchema = shallowCopy(true);
+ for (Map.Entry<String, Collection<String>> entry : copyFields.entrySet()) {
+ //Key is the name of the field, values are the destinations
- for (String destination : entry.getValue()) {
- newSchema.registerCopyField(entry.getKey(), destination);
- }
- }
- //TODO: move this common stuff out to shared methods
- // Run the callbacks on SchemaAware now that everything else is done
- for (SchemaAware aware : newSchema.schemaAware) {
- aware.inform(newSchema);
- }
- newSchema.refreshAnalyzers();
- success = newSchema.persistManagedSchema(false); // don't just create - update it if it already exists
- if (success) {
- log.debug("Added copy fields for {} sources", copyFields.size());
- } else {
- log.error("Failed to add copy fields for {} sources", copyFields.size());
+ for (String destination : entry.getValue()) {
+ newSchema.registerCopyField(entry.getKey(), destination);
}
}
+ //TODO: move this common stuff out to shared methods
+ // Run the callbacks on SchemaAware now that everything else is done
+ for (SchemaAware aware : newSchema.schemaAware) {
+ aware.inform(newSchema);
+ }
+ newSchema.refreshAnalyzers();
+ success = newSchema.persistManagedSchema(false); // don't just create - update it if it already exists
+ if (success) {
+ log.debug("Added copy fields for {} sources", copyFields.size());
+ } else {
+ log.error("Failed to add copy fields for {} sources", copyFields.size());
+ }
}
return newSchema;
}
@@ -431,7 +424,8 @@ public final class ManagedIndexSchema ex
return newSchema;
}
-
+
+ @Override
public Object getSchemaUpdateLock() {
return schemaUpdateLock;
}
Modified: lucene/dev/trunk/solr/core/src/java/org/apache/solr/update/processor/AddSchemaFieldsUpdateProcessorFactory.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/java/org/apache/solr/update/processor/AddSchemaFieldsUpdateProcessorFactory.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/java/org/apache/solr/update/processor/AddSchemaFieldsUpdateProcessorFactory.java (original)
+++ lucene/dev/trunk/solr/core/src/java/org/apache/solr/update/processor/AddSchemaFieldsUpdateProcessorFactory.java Tue Jul 8 03:20:54 2014
@@ -315,14 +315,16 @@ public class AddSchemaFieldsUpdateProces
log.debug(builder.toString());
}
try {
- IndexSchema newSchema = oldSchema.addFields(newFields);
- if (null != newSchema) {
- cmd.getReq().getCore().setLatestSchema(newSchema);
- cmd.getReq().updateSchemaToLatest();
- log.debug("Successfully added field(s) to the schema.");
- break; // success - exit from the retry loop
- } else {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Failed to add fields.");
+ synchronized (oldSchema.getSchemaUpdateLock()) {
+ IndexSchema newSchema = oldSchema.addFields(newFields);
+ if (null != newSchema) {
+ cmd.getReq().getCore().setLatestSchema(newSchema);
+ cmd.getReq().updateSchemaToLatest();
+ log.debug("Successfully added field(s) to the schema.");
+ break; // success - exit from the retry loop
+ } else {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Failed to add fields.");
+ }
}
} catch(ManagedIndexSchema.FieldExistsException e) {
log.debug("At least one field to be added already exists in the schema - retrying.");
Modified: lucene/dev/trunk/solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchemaConcurrent.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchemaConcurrent.java?rev=1608646&r1=1608645&r2=1608646&view=diff
==============================================================================
--- lucene/dev/trunk/solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchemaConcurrent.java (original)
+++ lucene/dev/trunk/solr/core/src/test/org/apache/solr/schema/TestCloudManagedSchemaConcurrent.java Tue Jul 8 03:20:54 2014
@@ -111,18 +111,19 @@ public class TestCloudManagedSchemaConcu
verifySuccess(request, response);
}
- private String[] getExpectedFieldResponses(int numAddFieldPuts, int numAddFieldPosts) {
+ private String[] getExpectedFieldResponses(int numAddFieldPuts, String putFieldName,
+ int numAddFieldPosts, String postFieldName) {
String[] expectedAddFields = new String[1 + numAddFieldPuts + numAddFieldPosts];
expectedAddFields[0] = SUCCESS_XPATH;
for (int i = 0; i < numAddFieldPuts; ++i) {
- String newFieldName = "newfieldPut" + i;
+ String newFieldName = putFieldName + i;
expectedAddFields[1 + i]
= "/response/arr[@name='fields']/lst/str[@name='name'][.='" + newFieldName + "']";
}
for (int i = 0; i < numAddFieldPosts; ++i) {
- String newFieldName = "newfieldPost" + i;
+ String newFieldName = postFieldName + i;
expectedAddFields[1 + numAddFieldPuts + i]
= "/response/arr[@name='fields']/lst/str[@name='name'][.='" + newFieldName + "']";
}
@@ -148,6 +149,11 @@ public class TestCloudManagedSchemaConcu
@Override
public void doTest() throws Exception {
setupHarnesses();
+ concurrentOperationsTest();
+ schemaLockTest();
+ }
+
+ private void concurrentOperationsTest() throws Exception {
// First, add a bunch of fields via PUT and POST, as well as copyFields,
// but do it fast enough and verify shards' schemas after all of them are added
@@ -155,16 +161,19 @@ public class TestCloudManagedSchemaConcu
int numAddFieldPuts = 0;
int numAddFieldPosts = 0;
List<CopyFieldInfo> copyFields = new ArrayList<>();
+
+ final String putFieldName = "newfieldPut";
+ final String postFieldName = "newfieldPost";
for (int i = 0; i <= numFields ; ++i) {
RestTestHarness publisher = restTestHarnesses.get(r.nextInt(restTestHarnesses.size()));
int type = random().nextInt(3);
if (type == 0) { // send an add field via PUT
- addFieldPut(publisher, "newfieldPut" + numAddFieldPuts++);
+ addFieldPut(publisher, putFieldName + numAddFieldPuts++);
}
else if (type == 1) { // send an add field via POST
- addFieldPost(publisher, "newfieldPost" + numAddFieldPosts++);
+ addFieldPost(publisher, postFieldName + numAddFieldPosts++);
}
else if (type == 2) { // send a copy field
String sourceField = null;
@@ -196,7 +205,8 @@ public class TestCloudManagedSchemaConcu
}
}
- String[] expectedAddFields = getExpectedFieldResponses(numAddFieldPuts, numAddFieldPosts);
+ String[] expectedAddFields = getExpectedFieldResponses(numAddFieldPuts, putFieldName,
+ numAddFieldPosts, postFieldName);
String[] expectedCopyFields = getExpectedCopyFieldResponses(copyFields);
boolean success = false;
@@ -236,6 +246,93 @@ public class TestCloudManagedSchemaConcu
}
}
+ private class PutPostThread extends Thread {
+ RestTestHarness harness;
+ String fieldName;
+ boolean isPut;
+ public PutPostThread(RestTestHarness harness, String fieldName, boolean isPut) {
+ this.harness = harness;
+ this.fieldName = fieldName;
+ this.isPut = isPut;
+ }
+
+ public void run() {
+ try {
+ if (isPut) {
+ addFieldPut(harness, fieldName);
+ } else {
+ addFieldPost(harness, fieldName);
+ }
+ } catch (Exception e) {
+ // log.error("###ACTUAL FAILURE!");
+ throw new RuntimeException(e);
+ }
+ }
+ }
+
+ private void schemaLockTest() throws Exception {
+
+ // First, add a bunch of fields via PUT and POST, as well as copyFields,
+ // but do it fast enough and verify shards' schemas after all of them are added
+ int numFields = 25;
+ int numAddFieldPuts = 0;
+ int numAddFieldPosts = 0;
+
+ final String putFieldName = "newfieldPutThread";
+ final String postFieldName = "newfieldPostThread";
+
+ for (int i = 0; i <= numFields ; ++i) {
+ // System.err.println("###ITERATION: " + i);
+ int postHarness = r.nextInt(restTestHarnesses.size());
+ RestTestHarness publisher = restTestHarnesses.get(postHarness);
+ PutPostThread postThread = new PutPostThread(publisher, postFieldName + numAddFieldPosts++, false);
+ postThread.start();
+
+ int putHarness = r.nextInt(restTestHarnesses.size());
+ publisher = restTestHarnesses.get(putHarness);
+ PutPostThread putThread = new PutPostThread(publisher, putFieldName + numAddFieldPuts++, true);
+ putThread.start();
+ postThread.join();
+ putThread.join();
+
+ String[] expectedAddFields = getExpectedFieldResponses(numAddFieldPuts, putFieldName,
+ numAddFieldPosts, postFieldName);
+
+ boolean success = false;
+ long maxTimeoutMillis = 100000;
+ long startTime = System.nanoTime();
+ String request = null;
+ String response = null;
+ String result = null;
+
+ while ( ! success
+ && TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTime, TimeUnit.NANOSECONDS) < maxTimeoutMillis) {
+ Thread.sleep(10);
+
+ // int j = 0;
+ for (RestTestHarness client : restTestHarnesses) {
+ // System.err.println("###CHECKING HARNESS: " + j++ + " for iteration: " + i);
+
+ // verify addFieldPuts and addFieldPosts
+ request = "/schema/fields?wt=xml";
+ response = client.query(request);
+ //System.err.println("###RESPONSE: " + response);
+ result = BaseTestHarness.validateXPath(response, expectedAddFields);
+ if (result != null) {
+ // System.err.println("###FAILURE!");
+ break;
+ }
+ }
+ success = (result == null);
+ }
+ if ( ! success) {
+ String msg = "QUERY FAILED: xpath=" + result + " request=" + request + " response=" + response;
+ log.error(msg);
+ fail(msg);
+ }
+ }
+ }
+
private static class CopyFieldInfo {
private String sourceField;
private String destField;