dsmiley commented on code in PR #4762:
URL: https://github.com/apache/solr/pull/4762#discussion_r3817536865
##########
solr/core/src/java/org/apache/solr/cli/RunExampleTool.java:
##########
@@ -673,8 +671,8 @@ protected void runCloudExample(CommandLine cli) throws
Exception {
/** wait until the number of live nodes == numNodes. */
protected void waitToSeeLiveNodes(String zkHost, int numNodes) {
- try (CloudSolrClient cloudClient =
- new CloudSolrClient.Builder(List.of(zkHost),
Optional.empty()).build()) {
+ // honours a chroot inside zkHost, e.g. zk1:2181/solr
Review Comment:
nice comment
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java:
##########
@@ -1204,7 +1204,7 @@ private void testAliasCreationNameValidation() throws
Exception {
private void testShardCreationNameValidation() throws Exception {
try (CloudSolrClient client = createCloudClient(null)) {
- client.connect();
+ client.getClusterStateProvider().getLiveNodes(); // force the connection
now
Review Comment:
Unnecessary, I presume
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java:
##########
@@ -737,7 +737,7 @@ private void clusterStatusBadCollectionTest() throws
Exception {
private void replicaPropTest() throws Exception {
try (CloudSolrClient client = createCloudClient(null)) {
- client.connect();
+ client.getClusterStateProvider().getLiveNodes(); // force the connection
now
Review Comment:
Unnecessary, I presume
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java:
##########
@@ -687,7 +687,7 @@ private void clusterStatusWithCollectionAndShardJSON()
throws IOException, SolrS
private void clusterStatusRolesTest() throws Exception {
try (CloudSolrClient client = createCloudClient(null)) {
- client.connect();
+ client.getClusterStateProvider().getLiveNodes(); // force the connection
now
Review Comment:
Unnecessary, I presume
##########
solr/core/src/test/org/apache/solr/util/tracing/TestSimplePropagatorDistributedTracing.java:
##########
@@ -170,11 +168,9 @@ private void assertSameTraceId(LogListener reqLog, String
traceId) {
}
private CloudSolrClient newCloudSolrClient() {
- var builder =
Review Comment:
the PR intro implied this `var` isn't what we think it is. Isn't it a
builder?
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -96,7 +96,8 @@ public class SolrCloudTestCase extends SolrTestCaseJ4 {
protected static SolrZkClient zkClient() {
ZkStateReader reader = cluster.getZkStateReader();
- if (reader == null) cluster.getSolrClient().connect();
+ // force the ZkStateReader into existence
+ if (reader == null)
cluster.getSolrClient().getClusterStateProvider().getLiveNodes();
Review Comment:
getZkStateReader will do that well enough
##########
solr/core/src/test/org/apache/solr/util/tracing/TestSimplePropagatorDistributedTracing.java:
##########
@@ -170,11 +168,9 @@ private void assertSameTraceId(LogListener reqLog, String
traceId) {
}
private CloudSolrClient newCloudSolrClient() {
- var builder =
- new CloudSolrClient.Builder(
- List.of(cluster.getZkServer().getZkAddress()), Optional.empty());
+ var builder = new
CloudSolrClient.Builder(cluster.getZkServer().getZkAddress());
Review Comment:
I've seen this so many times (and I've got a variant of this wrong before
and it bit me) that I feel cluster needs a CSC builder method to make this more
idiomatic. Any way, defer my comment if you wish.
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/sql/JdbcTest.java:
##########
@@ -646,7 +646,7 @@ private void testJDBCMethods(
}
CloudSolrClient solrClient = cluster.getSolrClient();
- solrClient.connect();
+ solrClient.getClusterStateProvider().getLiveNodes(); // force the
connection now
Review Comment:
Hmm; I suspect this isn't necessary. I believe obtaining the ZkStateReader
itself implies a connection.
##########
solr/core/src/test/org/apache/solr/cloud/OverseerTest.java:
##########
@@ -1808,7 +1808,7 @@ private SolrCloudManager
getCloudDataProvider(ZkStateReader zkStateReader) {
solrClients.add(cloudSolrClient);
solrClients.add(httpSolrClient);
SolrClientCloudManager sccm = new SolrClientCloudManager(cloudSolrClient,
null);
- sccm.getClusterStateProvider().connect();
+ sccm.getClusterStateProvider().getLiveNodes();
Review Comment:
either not needed or deserves a comment as to why it is
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/TestReplicaProperties.java:
##########
@@ -85,7 +85,7 @@ private void listCollection() throws IOException,
SolrServerException {
private void clusterAssignPropertyTest() throws Exception {
try (CloudSolrClient client = createCloudClient(null)) {
- client.connect();
+ client.getClusterStateProvider().getLiveNodes(); // force the connection
now
Review Comment:
Unnecessary, I presume
##########
solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java:
##########
@@ -545,7 +545,7 @@ private void assertFailure(Request req, String
expectedErrorSubstring) throws IO
}
private void assertCollectionExists(String name) {
- solrClient.getClusterStateProvider().connect(); // TODO get rid of this
+ solrClient.getClusterStateProvider().getLiveNodes(); // TODO get rid of
this
Review Comment:
Note the TODO and the URL. I believe the objective is to remove this line
and, if needed, harden the call chain below to ensure it doesn't NPE if it
hasn't yet internally connected. ~8 years ago Noble argued that's another
issue to do (that wasn't done?) but IMO it's so simple so why not commence now.
##########
solr/cross-dc-manager/src/java/org/apache/solr/crossdc/manager/messageprocessor/SolrMessageProcessor.java:
##########
@@ -377,7 +377,8 @@ private void connectToSolrIfNeeded() {
boolean connected = false;
while (!connected) {
try {
- clientSupplier.get().connect(); // volatile null-check if already
connected
+ // a cheap round-trip that fails the same way connect() did
Review Comment:
let's not speak of a non-existent method
##########
solr/solr-ref-guide/modules/deployment-guide/examples/UsingSolrJRefGuideExamplesTest.java:
##########
Review Comment:
love it
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -356,7 +356,7 @@ protected void initCloud() {
assert (cloudInit == false);
cloudInit = true;
cloudClient = createCloudClient(DEFAULT_COLLECTION);
- cloudClient.connect();
+ cloudClient.getClusterStateProvider().getLiveNodes(); // force the
connection now
Review Comment:
`ZkStateReader.from` will do that
--
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]
