janhoy commented on a change in pull request #708:
URL: https://github.com/apache/solr/pull/708#discussion_r824989316
##########
File path: solr/core/src/test/org/apache/solr/cloud/DeleteReplicaTest.java
##########
@@ -36,13 +36,7 @@
import org.apache.solr.cloud.overseer.OverseerAction;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.DocCollectionWatcher;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
-import org.apache.solr.common.cloud.ZkNodeProps;
-import org.apache.solr.common.cloud.ZkStateReader;
-import org.apache.solr.common.cloud.ZkStateReaderAccessor;
+import org.apache.solr.common.cloud.*;
Review comment:
Same comment here as above about wildcard imports
##########
File path: solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java
##########
@@ -38,10 +38,7 @@
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.cloud.SolrCloudManager;
import org.apache.solr.client.solrj.embedded.JettySolrRunner;
-import org.apache.solr.client.solrj.impl.CloudSolrClient;
-import org.apache.solr.client.solrj.impl.ClusterStateProvider;
-import org.apache.solr.client.solrj.impl.HttpSolrClient;
-import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.client.solrj.impl.*;
Review comment:
Think we encourage listing each import rather than `*`, you can change
your IDE setting
##########
File path: solr/core/src/test/org/apache/solr/cloud/ReplicationFactorTest.java
##########
@@ -36,10 +36,7 @@
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.DocRouter;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.ZkCoreNodeProps;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard import
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudDeleteByQuery.java
##########
@@ -128,7 +128,7 @@ private static void createMiniSolrCloudCluster() throws
Exception {
CLOUD_CLIENT = cluster.getSolrClient();
CLOUD_CLIENT.setDefaultCollection(COLLECTION_NAME);
- ZkStateReader zkStateReader = CLOUD_CLIENT.getZkStateReader();
+ ZkStateReader zkStateReader = ZkStateReader.from(cluster.getSolrClient());
Review comment:
```suggestion
ZkStateReader zkStateReader = ZkStateReader.from(CLOUD_CLIENT);
```
##########
File path:
solr/core/src/test/org/apache/solr/search/join/CrossCollectionJoinQueryTest.java
##########
@@ -197,7 +197,7 @@ public void testCcJoinRoutedCollection() throws Exception {
String.format(
Locale.ROOT,
"{!join method=crossCollection zkHost=\"%s\" fromIndex=products
from=product_id_s to=product_id_s}size_s:M",
- cluster.getSolrClient().getZkHost()),
+ client.getClusterStateProvider().getQuorumHosts()),
Review comment:
Think we need the same trick here with something like
```
((ZkClusterStateProvider)client.getClusterStateProvider()).zkHost),
```
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java
##########
@@ -100,4 +100,6 @@ default Object getClusterProperty(String propertyName) {
String getPolicyNameByCollection(String coll);
void connect();
+
+ String getQuorumHosts();
Review comment:
@haythemkh , @dsmiley Can you explain the main reason for this new
method - I see that a lot of places which earlier just fetched the *configured*
zk string, now calls this method, which for ZkClusterStateProvider requires an
initialized zk connection...
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
##########
@@ -358,4 +358,12 @@ public void setCacheTimeout(int cacheTimeout) {
// This exception is not meant to escape this class it should be caught and
wrapped.
private class NotACollectionException extends Exception {}
+
+ @Override
+ public String getQuorumHosts() {
+ if (this.liveNodes == null) {
+ return null;
+ }
+ return String.join(",", this.liveNodes);
Review comment:
Why is livenodes returned here? I thought QuorumHosts was the Zookeeper
hosts, not the solr hosts?
##########
File path:
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZkTestBase.java
##########
@@ -1505,9 +1509,10 @@ private void testANewCollectionInOneInstance() throws
Exception {
SolrClient client3 = collectionClients.get(2);
SolrClient client4 = collectionClients.get(3);
- waitForRecoveriesToFinish(
- oneInstanceCollection, getCommonCloudSolrClient().getZkStateReader(),
false);
- assertAllActive(oneInstanceCollection,
getCommonCloudSolrClient().getZkStateReader());
+ getCommonCloudSolrClient();
+ waitForRecoveriesToFinish(oneInstanceCollection,
ZkStateReader.from(cloudClient), false);
+ getCommonCloudSolrClient();
+ assertAllActive(oneInstanceCollection, ZkStateReader.from(cloudClient));
Review comment:
Can you explain why `getCommonCloudSolrClient()` is called twice here
and why the return value is not used?
##########
File path: solr/core/src/test/org/apache/solr/cloud/MigrateRouteKeyTest.java
##########
@@ -29,10 +29,7 @@
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.RoutingRule;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard import...
##########
File path:
solr/core/src/test/org/apache/solr/cloud/DistribDocExpirationUpdateProcessorTest.java
##########
@@ -36,9 +36,7 @@
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard import
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
##########
@@ -249,8 +248,9 @@ private void addDocWhenOtherReplicasAreNetworkPartitioned(
TimeoutException.class,
"Did not time out waiting for new leader, out of sync replica became
leader",
() -> {
- cluster
- .getSolrClient()
+ // this is is the bad case, our "bad" state was found before timeout
Review comment:
```suggestion
// this is the bad case, our "bad" state was found before timeout
```
##########
File path:
solr/core/src/test/org/apache/solr/cloud/TestLeaderElectionWithEmptyReplica.java
##########
@@ -28,9 +28,7 @@
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Get rid of wildcard import
##########
File path:
solr/core/src/test/org/apache/solr/cloud/TestTolerantUpdateProcessorCloud.java
##########
@@ -117,7 +117,7 @@ public static void createMiniSolrCloudCluster() throws
Exception {
cluster.waitForActiveCollection(COLLECTION_NAME, NUM_SHARDS,
REPLICATION_FACTOR * NUM_SHARDS);
- ZkStateReader zkStateReader = CLOUD_CLIENT.getZkStateReader();
+ ZkStateReader zkStateReader = ZkStateReader.from(cluster.getSolrClient());
Review comment:
```suggestion
ZkStateReader zkStateReader = ZkStateReader.from(CLOUD_CLIENT);
```
##########
File path:
solr/core/src/test/org/apache/solr/update/DeleteByIdWithRouterFieldTest.java
##########
@@ -33,10 +33,7 @@
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrDocumentList;
import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.ClusterState;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.cloud.*;
Review comment:
Wildcard...
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
##########
@@ -64,17 +64,7 @@
import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.ToleratedUpdateError;
-import org.apache.solr.common.cloud.ClusterState;
-import org.apache.solr.common.cloud.CollectionStatePredicate;
-import org.apache.solr.common.cloud.CollectionStateWatcher;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.DocCollectionWatcher;
-import org.apache.solr.common.cloud.DocRouter;
-import org.apache.solr.common.cloud.ImplicitDocRouter;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
-import org.apache.solr.common.cloud.ZkCoreNodeProps;
-import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.cloud.*;
Review comment:
Fix wildcard imports
##########
File path:
solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java
##########
@@ -46,20 +46,9 @@
import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.client.solrj.response.RequestStatusState;
-import org.apache.solr.cloud.AbstractDistribZkTestBase;
-import org.apache.solr.cloud.BasicDistributedZkTest;
-import org.apache.solr.cloud.SolrCloudTestCase;
-import org.apache.solr.cloud.StoppableIndexingThread;
+import org.apache.solr.cloud.*;
Review comment:
Fix wildcard imports
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java
##########
@@ -39,9 +39,9 @@
volatile ZkStateReader zkStateReader;
private boolean closeZkStateReader = true;
- String zkHost;
- int zkConnectTimeout = 15000;
- int zkClientTimeout = 45000;
+ public String zkHost;
+ public int zkConnectTimeout = 15000;
Review comment:
Without having looked, why the change to public here?
##########
File path:
solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java
##########
@@ -160,7 +160,7 @@ protected MetricSamples request(SolrClient client,
MetricsQuery query) throws IO
if (client instanceof CloudSolrClient) {
labelNames.add("zk_host");
- labelValues.add(((CloudSolrClient) client).getZkHost());
+ labelValues.add(((CloudSolrClient)
client).getClusterStateProvider().getQuorumHosts());
Review comment:
Should instead just get the zkHost string
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/cloud/DelegatingClusterStateProvider.java
##########
@@ -124,4 +124,12 @@ public void close() throws IOException {
delegate.close();
}
}
+
+ @Override
+ public String getQuorumHosts() {
+ if (delegate != null) {
+ return delegate.getQuorumHosts();
+ }
+ return null;
Review comment:
What does it mean to return null here? Should it instead throw exception?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
