[lucene-solr] branch branch_8x updated: Harden CollectionPropsTest:

[ho...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

hossman pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git


The following commit(s) were added to refs/heads/branch_8x by this push:
     new b54f436  Harden CollectionPropsTest:
b54f436 is described below

commit b54f43686a8b9a9691a69ff2cb42fda5de5e024b
Author: Chris Hostetter <ho...@apache.org>
AuthorDate: Fri Aug 2 16:59:41 2019 -0700

    Harden CollectionPropsTest:
    
    These fixes all relate to testWatcher + testMultipleWatchers:
    * add additional asserts to the test methods to assert the expected property values are found
    * mark Watcher.props volatile to prevent stale read by test thread
    * add some randomization to Watcher.props to either come from the onStateChanged() input or
      from an explicit call to ZkStateReader.getCollectionProperties
      - previuosly, for reasons i don't understand, the test only consulted
        ZkStateReader.getCollectionProperties inside the Watcher, and ignored the onStateChanged()
        input
      - now the test validates both
    * move all Watcher.triggered access into the existing synchronization blocks to prevent
      waitForTrigger() from returning prematurely due to gaining synch lock _after_
      Watcher.triggered was incremented in onStateChanged(), but _before_ onStateChanged() updated
      Watcher.props
    * add detailed logging to provide additional info to help debug any additional jenkins failures
      that might pop up in the future if these fixes aren't sufficient
    
    (cherry picked from commit e8418adedbcd0e64cbe53e9b7b935107ce24237a)
---
 .../org/apache/solr/cloud/CollectionPropsTest.java | 64 +++++++++++++++++-----
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/solr/core/src/test/org/apache/solr/cloud/CollectionPropsTest.java b/solr/core/src/test/org/apache/solr/cloud/CollectionPropsTest.java
index 5d9e4ee..9061e58 100644
--- a/solr/core/src/test/org/apache/solr/cloud/CollectionPropsTest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/CollectionPropsTest.java
@@ -21,6 +21,8 @@ import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.nio.charset.StandardCharsets;
 import java.util.Locale;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.lucene.util.LuceneTestCase;
@@ -168,27 +170,33 @@ public class CollectionPropsTest extends SolrCloudTestCase {
     CollectionProperties collectionProps = new CollectionProperties(zkClient());
 
     // Add a watcher to collection props
-    final Watcher watcher = new Watcher();
+    final Watcher watcher = new Watcher("Watcher", random().nextBoolean());
     zkStateReader.registerCollectionPropsWatcher(collectionName, watcher);
     assertEquals(0, watcher.waitForTrigger(TEST_NIGHTLY?2000:200));
 
     // Trigger a new znode event
+    log.info("setting value1");
     collectionProps.setCollectionProperty(collectionName, "property", "value1");
     assertEquals(1, watcher.waitForTrigger());
     assertEquals("value1", watcher.getProps().get("property"));
 
     // Trigger a value change event
+    log.info("setting value2");
     collectionProps.setCollectionProperty(collectionName, "property", "value2");
-    watcher.waitForTrigger();
+    log.info("(value2) waitForTrigger=={}", watcher.waitForTrigger());
     assertEquals("value2", watcher.getProps().get("property"));
 
     // Delete the properties znode
+    log.info("deleting props");
     zkStateReader.getZkClient().delete("/collections/" + collectionName + "/collectionprops.json", -1, true);
     assertEquals(1, watcher.waitForTrigger());
-    assertTrue(watcher.getProps().isEmpty());
+    final Map<String, String> props = watcher.getProps();
+    assertTrue(props.toString(), props.isEmpty());
 
     // Remove watcher and make sure that the watcher is not triggered
+    log.info("removing watcher");
     zkStateReader.removeCollectionPropsWatcher(collectionName, watcher);
+    log.info("setting value1 (again)");
     collectionProps.setCollectionProperty(collectionName, "property", "value1");
     assertEquals("ZK watcher was triggered after it was removed for collection " + collectionName, 0, watcher.waitForTrigger());
   }
@@ -202,49 +210,78 @@ public class CollectionPropsTest extends SolrCloudTestCase {
     zkStateReader.registerCore(collectionName);
 
     // Subsequent watchers won't be triggered when adding
-    final Watcher watcher1 = new Watcher();
+    final Watcher watcher1 = new Watcher("Watcher1", random().nextBoolean());
     zkStateReader.registerCollectionPropsWatcher(collectionName, watcher1);
     watcher1.waitForTrigger(); // this might still get triggered because of registerCore
-    final Watcher watcher2 = new Watcher();
+    final Watcher watcher2 = new Watcher("Watcher2", random().nextBoolean());
     zkStateReader.registerCollectionPropsWatcher(collectionName, watcher2);
     assertEquals(0, watcher2.waitForTrigger(TEST_NIGHTLY?2000:200));
 
     // Make sure a value change triggers both watchers
+    log.info("setting value1");
     collectionProps.setCollectionProperty(collectionName, "property", "value1");
     assertEquals(1, watcher1.waitForTrigger());
     assertEquals(1, watcher2.waitForTrigger());
+    assertEquals("value1", watcher1.getProps().get("property"));
+    assertEquals("value1", watcher2.getProps().get("property"));
 
     // The watchers should be triggered when after the core is unregistered
     zkStateReader.unregisterCore(collectionName);
+    log.info("setting value2");
     collectionProps.setCollectionProperty(collectionName, "property", "value2");
     assertEquals(1, watcher1.waitForTrigger());
+    assertEquals(1, watcher2.waitForTrigger());
+    assertEquals("value2", watcher1.getProps().get("property"));
+    assertEquals("value2", watcher2.getProps().get("property"));
 
     // The watcher should be triggered after another watcher is removed
+    log.info("removing watcher2");
     zkStateReader.removeCollectionPropsWatcher(collectionName, watcher2);
+    log.info("setting value3");
     collectionProps.setCollectionProperty(collectionName, "property", "value3");
     assertEquals(1, watcher1.waitForTrigger());
+    assertEquals("value3", watcher1.getProps().get("property"));
 
     // The last watcher shouldn't be triggered after removed, even if the core is registered
     zkStateReader.registerCore(collectionName);
+    log.info("removing watcher1");
     zkStateReader.removeCollectionPropsWatcher(collectionName, watcher1);
+    log.info("setting value4");
     collectionProps.setCollectionProperty(collectionName, "property", "value4");
     assertEquals(0, watcher1.waitForTrigger(TEST_NIGHTLY?2000:200));
   }
 
   private class Watcher implements CollectionPropsWatcher {
-    private Map<String, String> props = null;
-    private AtomicInteger triggered = new AtomicInteger();
-
+    private final String name;
+    private final boolean forceReadPropsFromZk;
+    private volatile Map<String, String> props = Collections.emptyMap();
+    private final AtomicInteger triggered = new AtomicInteger();
+
+    public Watcher(final String name, final boolean forceReadPropsFromZk) {
+      this.name = name;
+      this.forceReadPropsFromZk = forceReadPropsFromZk;
+      log.info("Watcher '{}' initialized with forceReadPropsFromZk={}", name, forceReadPropsFromZk);
+    }
+    
     @Override
     public boolean onStateChanged(Map<String, String> collectionProperties) {
-      triggered.incrementAndGet();
-      final ZkStateReader zkStateReader = cluster.getSolrClient().getZkStateReader();
-      props = zkStateReader.getCollectionProperties(collectionName);
+      log.info("{}: state changed...", name);
+      if (forceReadPropsFromZk) {
+        final ZkStateReader zkStateReader = cluster.getSolrClient().getZkStateReader();
+        props = Collections.unmodifiableMap(new HashMap(zkStateReader.getCollectionProperties(collectionName)));
+        log.info("{}: Setting props from zk={}", name, props);
+      } else {
+        props = Collections.unmodifiableMap(new HashMap(collectionProperties));
+        log.info("{}: Setting props from caller={}", name, props);
+      }
+      
       synchronized (this) {
+        triggered.incrementAndGet();
+        log.info("{}: notifying", name);
         notifyAll();
       }
 
-
+      log.info("{}: done", name);
       return false;
     }
 
@@ -263,9 +300,8 @@ public class CollectionPropsTest extends SolrCloudTestCase {
         }
 
         wait(waitTime);
+        return triggered.getAndSet(0);
       }
-
-      return triggered.getAndSet(0);
     }
   }
 }