dsmiley commented on code in PR #760:
URL: https://github.com/apache/solr/pull/760#discussion_r1817969516
##########
solr/core/src/java/org/apache/solr/cloud/ZkShardTerms.java:
##########
@@ -431,7 +424,7 @@ private void registerWatcher() throws KeeperException {
};
try {
// exists operation is faster than getData operation
- zkClient.exists(znodePath, watcher, true);
+ zkClient.getData(znodePath, watcher, null, true);
Review Comment:
see the comment :-)
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/DigestZkCredentialsProvider.java:
##########
@@ -34,15 +34,26 @@ public class DigestZkCredentialsProvider extends
DefaultZkCredentialsProvider {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
/** Called by reflective instantiation */
- public DigestZkCredentialsProvider() {}
+ public DigestZkCredentialsProvider() {
+ super();
+ }
Review Comment:
pointless change
##########
solr/core/src/test/org/apache/solr/cloud/ZkFailoverTest.java:
##########
@@ -31,6 +33,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+// Can be removed once CURATOR-720 is resolved, and the dependency is updated
+@ThreadLeakFilters(filters = {SolrIgnoredThreadsFilter.class})
Review Comment:
This is already declared on a superclass (SolrTestCase) , so should be
unnecessary
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/DigestZkACLProvider.java:
##########
@@ -39,7 +39,9 @@ public class DigestZkACLProvider extends
SecurityAwareZkACLProvider {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
/** Called by reflective instantiation */
- public DigestZkACLProvider() {}
+ public DigestZkACLProvider() {
+ super();
+ }
Review Comment:
pointless change
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -924,6 +993,20 @@ public String getZkServerAddress() {
return zkServerAddress;
}
+ /**
+ * @return the address of the zookeeper cluster
Review Comment:
no
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -781,14 +835,39 @@ public Stat setData(String path, Path source, boolean
retryOnConnLoss)
return setData(path, Files.readAllBytes(source), retryOnConnLoss);
}
- public List<OpResult> multi(final Iterable<Op> ops, boolean retryOnConnLoss)
- throws InterruptedException, KeeperException {
- List<OpResult> result = null;
- if (retryOnConnLoss) {
- result = zkCmdExecutor.retryOperation(() -> keeper.multi(ops));
- } else {
- result = keeper.multi(ops);
+ @FunctionalInterface
+ public interface CuratorOpBuilder {
Review Comment:
a line of javadocs for public classes would be helpful
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -37,6 +37,13 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
+
+ implementation('org.apache.curator:curator-client', {
+ exclude group: 'org.apache.zookeeper', module: 'zookeeper'
+ })
+ api('org.apache.curator:curator-framework', {
Review Comment:
implementation?
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/BadClusterTest.java:
##########
@@ -97,15 +102,15 @@ private void testAllNodesDown() throws Exception {
}
private void testClusterShutdown() throws Exception {
-
- CloudSolrStream stream = new CloudSolrStream(buildSearchExpression(),
streamFactory);
cluster.shutdown();
- try {
- getTuples(stream);
- fail("Expected IOException: SolrException: TimeoutException");
- } catch (IOException ioe) {
- SolrException se = (SolrException) ioe.getCause();
+ try (CloudSolrClient cloudSolrClient =
+ new CloudHttp2SolrClient.Builder(Collections.singletonList(zkHost),
Optional.empty())
+ .withZkConnectTimeout(1, TimeUnit.SECONDS)
+ .build()) {
+ cloudSolrClient.getById(collection, "1");
Review Comment:
the point was to use stream API I suppose
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -924,6 +993,20 @@ public String getZkServerAddress() {
return zkServerAddress;
}
+ /**
+ * @return the address of the zookeeper cluster
+ */
+ public String getChroot() {
+ return client.getNamespace();
+ }
+
+ /**
+ * @return the address of the zookeeper cluster
Review Comment:
huh?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/SolrZkClient.java:
##########
@@ -73,11 +81,8 @@ public class SolrZkClient implements Closeable {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private ConnectionManager connManager;
-
- private volatile ZooKeeper keeper;
-
- private ZkCmdExecutor zkCmdExecutor;
+ private ExecutorService curatorSafeServiceExecutor;
+ CuratorFramework client;
Review Comment:
not private?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
