lucene-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dsmi...@apache.org
Subject lucene-solr:branch_7x: SOLR-12256: AliasesManager.update() should call ZooKeeper.sync() * SetAliasPropCmd now calls AliasesManager.update() first. * SetAliasPropCmd now more efficiently updates multiple values. * Tests: Commented out BadApple annotations
Date Fri, 20 Apr 2018 20:24:13 GMT
Repository: lucene-solr
Updated Branches:
  refs/heads/branch_7x 959222119 -> b16b380b2


SOLR-12256: AliasesManager.update() should call ZooKeeper.sync()
* SetAliasPropCmd now calls AliasesManager.update() first.
* SetAliasPropCmd now more efficiently updates multiple values.
* Tests: Commented out BadApple annotations on alias related stuff.

(cherry picked from commit 8f296d0)


Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/b16b380b
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/b16b380b
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/b16b380b

Branch: refs/heads/branch_7x
Commit: b16b380b2cec1614597df6a045599307628988c2
Parents: 9592221
Author: David Smiley <dsmiley@apache.org>
Authored: Fri Apr 20 16:22:16 2018 -0400
Committer: David Smiley <dsmiley@apache.org>
Committed: Fri Apr 20 16:23:58 2018 -0400

----------------------------------------------------------------------
 solr/CHANGES.txt                                |  2 +
 .../cloud/api/collections/SetAliasPropCmd.java  | 45 +++++++++++---------
 .../apache/solr/cloud/AliasIntegrationTest.java | 33 +++++++-------
 .../solr/cloud/CreateRoutedAliasTest.java       | 10 +++--
 .../TimeRoutedAliasUpdateProcessorTest.java     | 31 ++++++++------
 .../apache/solr/common/cloud/ZkStateReader.java | 22 ++++++----
 6 files changed, 78 insertions(+), 65 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/b16b380b/solr/CHANGES.txt
----------------------------------------------------------------------
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index c21d3f1..e791e26 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -239,6 +239,8 @@ Bug Fixes
 
 * SOLR-12204: Upgrade commons-fileupload dependency to 1.3.3 to address CVE-2016-1000031.
 (Steve Rowe)
 
+* SOLR-12256: Fixed some eventual-consistency issues with collection aliases by using ZooKeeper.sync().
(David Smiley)
+
 ==================  7.3.0 ==================
 
 Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release.

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/b16b380b/solr/core/src/java/org/apache/solr/cloud/api/collections/SetAliasPropCmd.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/SetAliasPropCmd.java
b/solr/core/src/java/org/apache/solr/cloud/api/collections/SetAliasPropCmd.java
index 618b72d..fdee1d1 100644
--- a/solr/core/src/java/org/apache/solr/cloud/api/collections/SetAliasPropCmd.java
+++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/SetAliasPropCmd.java
@@ -18,6 +18,7 @@
 package org.apache.solr.cloud.api.collections;
 
 import java.lang.invoke.MethodHandles;
+import java.util.LinkedHashMap;
 import java.util.Locale;
 import java.util.Map;
 
@@ -29,7 +30,7 @@ import org.apache.solr.common.util.NamedList;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import static org.apache.solr.cloud.api.collections.OverseerCollectionMessageHandler.*;
+import static org.apache.solr.cloud.api.collections.OverseerCollectionMessageHandler.Cmd;
 import static org.apache.solr.common.SolrException.ErrorCode.BAD_REQUEST;
 import static org.apache.solr.common.params.CommonParams.NAME;
 
@@ -49,33 +50,35 @@ public class SetAliasPropCmd implements Cmd {
   public void call(ClusterState state, ZkNodeProps message, NamedList results) throws Exception
{
     String aliasName = message.getStr(NAME);
 
+    final ZkStateReader.AliasesManager aliasesManager = messageHandler.zkStateReader.aliasesManager;
 
-    ZkStateReader zkStateReader = messageHandler.zkStateReader;
-    if (zkStateReader.getAliases().getCollectionAliasMap().get(aliasName) == null) {
+    // Ensure we see the alias.  This may be redundant but SetAliasPropCmd isn't expected
to be called very frequently
+    aliasesManager.update();
+
+    if (aliasesManager.getAliases().getCollectionAliasMap().get(aliasName) == null) {
       // nicer than letting aliases object throw later on...
       throw new SolrException(BAD_REQUEST,
           String.format(Locale.ROOT,  "Can't modify non-existent alias %s", aliasName));
     }
 
     @SuppressWarnings("unchecked")
-    Map<String, String> properties = (Map<String, String>) message.get(PROPERTIES);
-
-    zkStateReader.aliasesManager.applyModificationAndExportToZk(aliases1 -> {
-      for (Map.Entry<String, String> entry : properties.entrySet()) {
-        String key = entry.getKey();
-        if ("".equals(key.trim())) {
-          throw new SolrException(BAD_REQUEST, "property keys must not be pure whitespace");
-        }
-        if (!key.equals(key.trim())) {
-          throw new SolrException(BAD_REQUEST, "property keys should not begin or end with
whitespace");
-        }
-        String value = entry.getValue();
-        if ("".equals(value)) {
-          value = null;
-        }
-        aliases1 = aliases1.cloneWithCollectionAliasProperties(aliasName, key, value);
+    Map<String, String> properties = new LinkedHashMap<>((Map<String, String>)
message.get(PROPERTIES));
+
+    // check & cleanup properties.  It's a mutable copy.
+    for (Map.Entry<String, String> entry : properties.entrySet()) {
+      String key = entry.getKey();
+      if ("".equals(key.trim())) {
+        throw new SolrException(BAD_REQUEST, "property keys must not be pure whitespace");
+      }
+      if (!key.equals(key.trim())) {
+        throw new SolrException(BAD_REQUEST, "property keys should not begin or end with
whitespace");
       }
-      return aliases1;
-    });
+      String value = entry.getValue();
+      if ("".equals(value)) {
+        entry.setValue(null);
+      }
+    }
+
+    aliasesManager.applyModificationAndExportToZk(aliases1 -> aliases1.cloneWithCollectionAliasProperties(aliasName,
properties));
   }
 }

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/b16b380b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java
index 9858ea7..2a82894 100644
--- a/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java
@@ -93,7 +93,7 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
   }
 
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028")
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028")
   public void testProperties() throws Exception {
     CollectionAdminRequest.createCollection("collection1meta", "conf", 2, 1).process(cluster.getSolrClient());
     CollectionAdminRequest.createCollection("collection2meta", "conf", 1, 1).process(cluster.getSolrClient());
@@ -118,16 +118,16 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
     assertTrue(((Map<String,Map<String,?>>)Utils.fromJSON(rawBytes)).get("collection").get("meta1")
instanceof String);
 
     // set properties
-    UnaryOperator<Aliases> op5 = a -> a.cloneWithCollectionAliasProperties("meta1",
"foo", "bar");
-    aliasesManager.applyModificationAndExportToZk(op5);
+    aliasesManager.applyModificationAndExportToZk(a1 ->
+        a1.cloneWithCollectionAliasProperties("meta1", "foo", "bar"));
     Map<String, String> meta = zkStateReader.getAliases().getCollectionAliasProperties("meta1");
     assertNotNull(meta);
     assertTrue(meta.containsKey("foo"));
     assertEquals("bar", meta.get("foo"));
 
     // set more properties
-    UnaryOperator<Aliases> op4 = a -> a.cloneWithCollectionAliasProperties("meta1",
"foobar", "bazbam");
-    aliasesManager.applyModificationAndExportToZk(op4);
+    aliasesManager.applyModificationAndExportToZk( a1 ->
+        a1.cloneWithCollectionAliasProperties("meta1", "foobar", "bazbam"));
     meta = zkStateReader.getAliases().getCollectionAliasProperties("meta1");
     assertNotNull(meta);
 
@@ -140,8 +140,8 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
     assertEquals("bazbam", meta.get("foobar"));
 
     // remove properties
-    UnaryOperator<Aliases> op3 = a -> a.cloneWithCollectionAliasProperties("meta1",
"foo", null);
-    aliasesManager.applyModificationAndExportToZk(op3);
+    aliasesManager.applyModificationAndExportToZk(a1 ->
+        a1.cloneWithCollectionAliasProperties("meta1", "foo", null));
     meta = zkStateReader.getAliases().getCollectionAliasProperties("meta1");
     assertNotNull(meta);
 
@@ -153,18 +153,17 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
     assertEquals("bazbam", meta.get("foobar"));
 
     // removal of non existent key should succeed.
-    UnaryOperator<Aliases> op2 = a -> a.cloneWithCollectionAliasProperties("meta1",
"foo", null);
-    aliasesManager.applyModificationAndExportToZk(op2);
+    aliasesManager.applyModificationAndExportToZk(a2 ->
+        a2.cloneWithCollectionAliasProperties("meta1", "foo", null));
 
     // chained invocations
-    UnaryOperator<Aliases> op1 = a ->
-        a.cloneWithCollectionAliasProperties("meta1", "foo2", "bazbam")
-        .cloneWithCollectionAliasProperties("meta1", "foo3", "bazbam2");
-    aliasesManager.applyModificationAndExportToZk(op1);
+    aliasesManager.applyModificationAndExportToZk(a1 ->
+        a1.cloneWithCollectionAliasProperties("meta1", "foo2", "bazbam")
+        .cloneWithCollectionAliasProperties("meta1", "foo3", "bazbam2"));
 
     // some other independent update (not overwritten)
-    UnaryOperator<Aliases> op = a -> a.cloneWithCollectionAlias("meta3", "collection1meta,collection2meta");
-    aliasesManager.applyModificationAndExportToZk(op);
+    aliasesManager.applyModificationAndExportToZk(a1 ->
+        a1.cloneWithCollectionAlias("meta3", "collection1meta,collection2meta"));
 
     // competing went through
     assertEquals("collection1meta,collection2meta", zkStateReader.getAliases().getCollectionAliasMap().get("meta3"));
@@ -240,7 +239,7 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
   }
 
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
   public void testModifyPropertiesV1() throws Exception {
     // note we don't use TZ in this test, thus it's UTC
     final String aliasName = getTestName();
@@ -256,7 +255,7 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
   }
 
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028")
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028")
   public void testModifyPropertiesCAR() throws Exception {
     // note we don't use TZ in this test, thus it's UTC
     final String aliasName = getTestName();

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/b16b380b/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java b/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
index 92135d6..4b81445 100644
--- a/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java
@@ -99,7 +99,7 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
   // This is a fairly complete test where we set many options and see that it both affected
the created
   //  collection and that the alias metadata was saved accordingly
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
   public void testV2() throws Exception {
     // note we don't use TZ in this test, thus it's UTC
     final String aliasName = getTestName();
@@ -181,7 +181,7 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
   }
 
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
   public void testV1() throws Exception {
     final String aliasName = getTestName();
     final String baseUrl = cluster.getRandomJetty(random()).getBaseUrl().toString();
@@ -225,7 +225,7 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
 
   // TZ should not affect the first collection name if absolute date given for start
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
   public void testTimezoneAbsoluteDate() throws Exception {
     final String aliasName = getTestName();
     try (SolrClient client = getCloudSolrClient(cluster)) {
@@ -244,7 +244,7 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
   }
 
   @Test
-  @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
+  //@BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 09-Apr-2018
   public void testCollectionNamesMustBeAbsent() throws Exception {
     CollectionAdminRequest.createCollection("collection1meta", "_default", 2, 1).process(cluster.getSolrClient());
     CollectionAdminRequest.createCollection("collection2meta", "_default", 1, 1).process(cluster.getSolrClient());
@@ -330,6 +330,7 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
         "&create-collection.numShards=1");
     assertFailure(get, "Unit not recognized");
   }
+
   @Test
   public void testNegativeFutureFails() throws Exception {
     final String aliasName = getTestName();
@@ -346,6 +347,7 @@ public class CreateRoutedAliasTest extends SolrCloudTestCase {
         "&create-collection.numShards=1");
     assertFailure(get, "must be >= 0");
   }
+
   @Test
   public void testUnParseableFutureFails() throws Exception {
     final String aliasName = "testAlias";

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/b16b380b/solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java
b/solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java
index ef8e1a5..cc7b7ce 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/TimeRoutedAliasUpdateProcessorTest.java
@@ -37,7 +37,6 @@ import org.apache.solr.client.solrj.impl.CloudSolrClient;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.ConfigSetAdminRequest;
 import org.apache.solr.client.solrj.request.V2Request;
-import org.apache.solr.client.solrj.response.ConfigSetAdminResponse;
 import org.apache.solr.client.solrj.response.FieldStatsInfo;
 import org.apache.solr.client.solrj.response.QueryResponse;
 import org.apache.solr.client.solrj.response.UpdateResponse;
@@ -71,6 +70,11 @@ public class TimeRoutedAliasUpdateProcessorTest extends SolrCloudTestCase
{
   public static void setupCluster() throws Exception {
     configureCluster(2).configure();
     solrClient = getCloudSolrClient(cluster);
+    //log this to help debug potential causes of problems
+    System.out.println("SolrClient: " + solrClient);
+    if (solrClient instanceof CloudSolrClient) {
+      System.out.println(((CloudSolrClient)solrClient).getClusterStateProvider());
+    }
   }
 
   @AfterClass
@@ -85,16 +89,17 @@ public class TimeRoutedAliasUpdateProcessorTest extends SolrCloudTestCase
{
     // Then we create a collection with the name of the eventual config.
     // We configure it, and ultimately delete the collection, leaving a modified config-set
behind.
     // Then when we create the "real" collections referencing this modified config-set.
-    final ConfigSetAdminRequest.Create adminRequest = new ConfigSetAdminRequest.Create();
-        adminRequest.setConfigSetName(configName);
-        adminRequest.setBaseConfigSetName("_default");
-        ConfigSetAdminResponse adminResponse = adminRequest.process(solrClient);
-        assertEquals(adminResponse.getStatus(), 0);
+    assertEquals(0, new ConfigSetAdminRequest.Create()
+        .setConfigSetName(configName)
+        .setBaseConfigSetName("_default")
+        .process(solrClient).getStatus());
 
-    CollectionAdminRequest.createCollection(configName, configName,1, 1).process(solrClient);
-    // manipulate the config...
+    CollectionAdminRequest.createCollection(configName, configName, 1, 1).process(solrClient);
 
-        String conf = "{" +
+    // manipulate the config...
+    checkNoError(solrClient.request(new V2Request.Builder("/collections/" + configName +
"/config")
+        .withMethod(SolrRequest.METHOD.POST)
+        .withPayload("{" +
             "  'set-user-property' : {'update.autoCreateFields':false}," + // no data driven
             "  'add-updateprocessor' : {" +
             "    'name':'tolerant', 'class':'solr.TolerantUpdateProcessorFactory'" +
@@ -103,10 +108,8 @@ public class TimeRoutedAliasUpdateProcessorTest extends SolrCloudTestCase
{
             "    'name':'inc', 'class':'" + IncrementURPFactory.class.getName() + "'," +
             "    'fieldName':'" + intField + "'" +
             "  }," +
-            "}";
-    checkNoError(solrClient.request(new V2Request.Builder("/collections/" + configName +
"/config")
-        .withMethod(SolrRequest.METHOD.POST)
-        .withPayload(conf).build()));    // only sometimes test with "tolerant" URP
+            "}").build()));
+    // only sometimes test with "tolerant" URP:
     final String urpNames = "inc" + (random().nextBoolean() ? ",tolerant" : "");
     checkNoError(solrClient.request(new V2Request.Builder("/collections/" + configName +
"/config/params")
         .withMethod(SolrRequest.METHOD.POST)
@@ -115,8 +118,8 @@ public class TimeRoutedAliasUpdateProcessorTest extends SolrCloudTestCase
{
             "    '_UPDATE' : {'processor':'" + urpNames + "'}" +
             "  }" +
             "}").build()));
-    CollectionAdminRequest.deleteCollection(configName).process(solrClient);
 
+    CollectionAdminRequest.deleteCollection(configName).process(solrClient);
     assertTrue(
         new ConfigSetAdminRequest.List().process(solrClient).getConfigSets()
             .contains(configName)

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/b16b380b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
----------------------------------------------------------------------
diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
index cfae849..9f1ddc6 100644
--- a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
+++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
@@ -16,14 +16,6 @@
  */
 package org.apache.solr.common.cloud;
 
-import static java.util.Arrays.asList;
-import static java.util.Collections.EMPTY_MAP;
-import static java.util.Collections.emptyMap;
-import static java.util.Collections.emptySet;
-import static java.util.Collections.emptySortedSet;
-import static java.util.Collections.unmodifiableSet;
-import static org.apache.solr.common.util.Utils.fromJSON;
-
 import java.io.Closeable;
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
@@ -51,6 +43,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
+
 import org.apache.solr.client.solrj.cloud.autoscaling.AutoScalingConfig;
 import org.apache.solr.common.Callable;
 import org.apache.solr.common.SolrException;
@@ -69,6 +62,14 @@ import org.apache.zookeeper.data.Stat;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import static java.util.Arrays.asList;
+import static java.util.Collections.EMPTY_MAP;
+import static java.util.Collections.emptyMap;
+import static java.util.Collections.emptySet;
+import static java.util.Collections.emptySortedSet;
+import static java.util.Collections.unmodifiableSet;
+import static org.apache.solr.common.util.Utils.fromJSON;
+
 public class ZkStateReader implements Closeable {
   public static final int STATE_UPDATE_DELAY = Integer.getInteger("solr.OverseerStateUpdateDelay",
2000);  // delay between cloud state updates
   private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
@@ -1705,7 +1706,7 @@ public class ZkStateReader implements Closeable {
             LOG.debug(e.toString(), e);
             LOG.warn("Couldn't save aliases due to race with another modification; will update
and retry until timeout");
             // considered a backoff here, but we really do want to compete strongly since
the normal case is
-            // that we will do one update and succeed. This is left as a hot loop for 5 tries
intentionally.
+            // that we will do one update and succeed. This is left as a hot loop for limited
tries intentionally.
             // More failures than that here probably indicate a bug or a very strange high
write frequency usage for
             // aliases.json, timeouts mean zk is being very slow to respond, or this node
is being crushed
             // by other processing and just can't find any cpu cycles at all.
@@ -1733,6 +1734,9 @@ public class ZkStateReader implements Closeable {
      * @return true if an update was performed
      */
     public boolean update() throws KeeperException, InterruptedException {
+      LOG.debug("Checking ZK for most up to date Aliases " + ALIASES);
+      // Call sync() first to ensure the subsequent read (getData) is up to date.
+      zkClient.getSolrZooKeeper().sync(ALIASES, null, null);
       Stat stat = new Stat();
       final byte[] data = zkClient.getData(ALIASES, null, stat, true);
       return setIfNewer(Aliases.fromJSON(data, stat.getVersion()));


Mime
View raw message