madrob commented on a change in pull request #2042:
URL: https://github.com/apache/lucene-solr/pull/2042#discussion_r513484808
##########
File path:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java
##########
@@ -259,12 +260,16 @@ public static void clean(SolrZkClient zkClient, String
path, Predicate<String> f
clean(zkClient, path);
return;
}
-
- TreeSet<String> paths = new
TreeSet<>(Comparator.comparingInt(String::length).reversed());
+
+ ArrayList<String> paths = new ArrayList<>();
+// TreeSet<String> paths = new
TreeSet<>(Comparator.comparingInt(String::length).reversed());
Review comment:
Please don't leave this commented out line in the code.
##########
File path: .gitignore
##########
@@ -25,3 +25,7 @@ __pycache__
# Emacs backup
*~
+/.caches/
+/eclipse-build/
+/lucene/
Review comment:
I... don't think this is right to include `/lucene/` and `/solr/`
##########
File path:
solr/solrj/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java
##########
@@ -220,4 +223,45 @@ public void testCheckInterrupted() {
SolrZkClient.checkInterrupted(new InterruptedException());
assertTrue(Thread.currentThread().isInterrupted());
}
+
+ /**
+ * This test reproduces the issue of trying to delete zk-nodes that have the
same length. (SOLR-14961).
+ *
+ * @throws InterruptedException when having troublke creating test nodes
+ * @throws KeeperException error when talking to zookeeper
+ * @throws SolrServerException when having trouble connecting to solr
+ * @throws UnsupportedEncodingException when getBytes() uses unknown encoding
+ *
+ */
+ @Test
+ public void testClean() throws KeeperException, InterruptedException,
SolrServerException, UnsupportedEncodingException {
Review comment:
A better place might be TestZkMaintenanceUtils
##########
File path:
solr/solrj/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java
##########
@@ -220,4 +223,45 @@ public void testCheckInterrupted() {
SolrZkClient.checkInterrupted(new InterruptedException());
assertTrue(Thread.currentThread().isInterrupted());
}
+
+ /**
+ * This test reproduces the issue of trying to delete zk-nodes that have the
same length. (SOLR-14961).
+ *
+ * @throws InterruptedException when having troublke creating test nodes
+ * @throws KeeperException error when talking to zookeeper
+ * @throws SolrServerException when having trouble connecting to solr
+ * @throws UnsupportedEncodingException when getBytes() uses unknown encoding
+ *
+ */
+ @Test
+ public void testClean() throws KeeperException, InterruptedException,
SolrServerException, UnsupportedEncodingException {
+ /* PREPARE */
+ String path = "/myPath/isTheBest";
+ String data1 = "myStringData1";
+ String data2 = "myStringData2";
+ String longData = "myLongStringData";
+ // create zk nodes that have the same path length
+ defaultClient.create("/myPath", null, CreateMode.PERSISTENT, true);
+ defaultClient.create(path, null, CreateMode.PERSISTENT, true);
+ defaultClient.create(path +"/file1.txt", data1.getBytes("UTF-8"),
CreateMode.PERSISTENT, true);
Review comment:
nit: use StandardCharsets.UTF8
##########
File path:
solr/solrj/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java
##########
@@ -259,12 +260,16 @@ public static void clean(SolrZkClient zkClient, String
path, Predicate<String> f
clean(zkClient, path);
return;
}
-
- TreeSet<String> paths = new
TreeSet<>(Comparator.comparingInt(String::length).reversed());
+
+ ArrayList<String> paths = new ArrayList<>();
+// TreeSet<String> paths = new
TreeSet<>(Comparator.comparingInt(String::length).reversed());
traverseZkTree(zkClient, path, VISIT_ORDER.VISIT_POST, znode -> {
if (!znode.equals("/") && filter.test(znode)) paths.add(znode);
});
+
+ // sort the list in descending order to ensure that child entries are
deleted first
Review comment:
Thank you for including this comment! It's really helpful to me!
##########
File path:
solr/solrj/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java
##########
@@ -220,4 +223,45 @@ public void testCheckInterrupted() {
SolrZkClient.checkInterrupted(new InterruptedException());
assertTrue(Thread.currentThread().isInterrupted());
}
+
+ /**
+ * This test reproduces the issue of trying to delete zk-nodes that have the
same length. (SOLR-14961).
+ *
+ * @throws InterruptedException when having troublke creating test nodes
+ * @throws KeeperException error when talking to zookeeper
+ * @throws SolrServerException when having trouble connecting to solr
+ * @throws UnsupportedEncodingException when getBytes() uses unknown encoding
+ *
+ */
+ @Test
+ public void testClean() throws KeeperException, InterruptedException,
SolrServerException, UnsupportedEncodingException {
+ /* PREPARE */
+ String path = "/myPath/isTheBest";
+ String data1 = "myStringData1";
+ String data2 = "myStringData2";
+ String longData = "myLongStringData";
+ // create zk nodes that have the same path length
+ defaultClient.create("/myPath", null, CreateMode.PERSISTENT, true);
+ defaultClient.create(path, null, CreateMode.PERSISTENT, true);
+ defaultClient.create(path +"/file1.txt", data1.getBytes("UTF-8"),
CreateMode.PERSISTENT, true);
+ defaultClient.create(path +"/nothing.txt", null, CreateMode.PERSISTENT,
true);
+ defaultClient.create(path +"/file2.txt", data2.getBytes("UTF-8"),
CreateMode.PERSISTENT, true);
+ defaultClient.create(path +"/some_longer_file2.txt",
longData.getBytes("UTF-8"), CreateMode.PERSISTENT, true);
+
+ String listZnode = defaultClient.listZnode(path, false);
+ log.info(listZnode);
+
+ /* RUN */
+ // delete all nodes that contain "file"
+ defaultClient.clean(path, node -> node.contains("file"));
+
+ /* CHECK */
+ listZnode = defaultClient.listZnode(path, false);
+ log.info(listZnode);
+ // list of node must not contain file1, file2 or some_longer_file2 because
they where deleted
+ assertTrue(!listZnode.contains("file1"));
Review comment:
nit: assertFalse
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
