dsmiley commented on code in PR #2935:
URL: https://github.com/apache/solr/pull/2935#discussion_r1903643954
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -65,6 +68,8 @@ public void init(List<String> solrUrls) throws Exception {
urlScheme = solrUrl.startsWith("https") ? "https" : "http";
try (SolrClient initialClient = getSolrClient(solrUrl)) {
this.liveNodes = fetchLiveNodes(initialClient);
+ this.initialNodes = Set.copyOf(liveNodes);
Review Comment:
I think the idea of this JIRA issue is that we'll take the Solr URLs as
configured and use *this* is the initial / backup liveNodes. I think this is a
very simple idea to to document/understand/implement.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -51,7 +52,9 @@ public abstract class BaseHttpClusterStateProvider implements
ClusterStateProvid
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private String urlScheme;
+ private Set<String> initialNodes;
volatile Set<String> liveNodes;
+ volatile Set<String> knownNodes;
Review Comment:
right away, I'm surprised. I can understand needing two sets, but 3? IMO
knownNodes doesn't need to exists; that's the same as liveNodes. I see below
you've changed many references from liveNodes to knownNodes but I recommend
against doing that because the exact wording of "liveNodes" is extremely
pervasive in SolrCloud (well known concept) so let's not have a vary it in just
this class or any class.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -34,15 +35,20 @@
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.DocCollection;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.embedded.JettySolrRunner;
import org.hamcrest.Matchers;
+import org.junit.After;
import org.junit.BeforeClass;
import org.junit.Test;
public class ClusterStateProviderTest extends SolrCloudTestCase {
+ private static JettySolrRunner jettyNode1;
+ private static JettySolrRunner jettyNode2;
Review Comment:
static fields in tests that refer to Solr should be null'ed out, so there's
some burden to them. Here... I think you could avoid these altogether and
simply call `cluster.getJettySolrRunner(0)` which isn't bad!
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -183,4 +204,99 @@ public void testClusterStateProvider() throws
SolrServerException, IOException {
clusterStateZk.getCollection("col2"),
equalTo(clusterStateHttp.getCollection("col2")));
}
}
+
+ @Test
+ public void testClusterStateProviderDownedLiveNodes() throws Exception {
+ try (var cspZk = zkClientClusterStateProvider();
+ var cspHttp = http2ClusterStateProvider()) {
+ Set<String> expectedLiveNodes = cspZk.getClusterState().getLiveNodes();
+ Set<String> actualLiveNodes = cspHttp.getLiveNodes();
+ assertEquals(2, actualLiveNodes.size());
+ assertEquals(expectedLiveNodes, actualLiveNodes);
+
+ cluster.stopJettySolrRunner(jettyNode1);
+ waitForCSPCacheTimeout();
+
+ expectedLiveNodes = cspZk.getClusterState().getLiveNodes();
+ actualLiveNodes = cspHttp.getLiveNodes();
+ assertEquals(1, actualLiveNodes.size());
+ assertEquals(expectedLiveNodes, actualLiveNodes);
+
+ cluster.startJettySolrRunner(jettyNode1);
+ cluster.stopJettySolrRunner(jettyNode2);
+ waitForCSPCacheTimeout();
+
+ // Should still be reachable because known hosts doesn't remove initial
nodes
+ expectedLiveNodes = cspZk.getClusterState().getLiveNodes();
+ actualLiveNodes = cspHttp.getLiveNodes();
+ assertEquals(1, actualLiveNodes.size());
+ assertEquals(expectedLiveNodes, actualLiveNodes);
+ }
+ }
+
+ @Test
+ public void testClusterStateProviderDownedKnownHosts() throws Exception {
+
+ try (var cspHttp = http2ClusterStateProvider()) {
+
+ String jettyNode1Url =
normalizeJettyUrl(jettyNode1.getBaseUrl().toString());
+ String jettyNode2Url =
normalizeJettyUrl(jettyNode2.getBaseUrl().toString());
+ Set<String> expectedKnownNodes = Set.of(jettyNode1Url, jettyNode2Url);
+ Set<String> actualKnownNodes = cspHttp.getKnownNodes();
+
+ assertEquals(2, actualKnownNodes.size());
+ assertEquals(expectedKnownNodes, actualKnownNodes);
+
+ cluster.stopJettySolrRunner(jettyNode1);
+ waitForCSPCacheTimeout();
+
+ // Known hosts should never remove the initial set of live nodes
+ actualKnownNodes = cspHttp.getKnownNodes();
+ assertEquals(2, actualKnownNodes.size());
+ assertEquals(expectedKnownNodes, actualKnownNodes);
+ }
+ }
+
+ @Test
+ public void testClusterStateProviderKnownHostsWithNewHost() throws Exception
{
+
+ try (var cspHttp = http2ClusterStateProvider()) {
+
+ var jettyNode3 = cluster.startJettySolrRunner();
+ String jettyNode1Url =
normalizeJettyUrl(jettyNode1.getBaseUrl().toString());
+ String jettyNode2Url =
normalizeJettyUrl(jettyNode2.getBaseUrl().toString());
+ String jettyNode3Url =
normalizeJettyUrl(jettyNode3.getBaseUrl().toString());
+ Set<String> expectedKnownNodes = Set.of(jettyNode1Url, jettyNode2Url,
jettyNode3Url);
+ waitForCSPCacheTimeout();
+
+ Set<String> actualKnownNodes = cspHttp.getKnownNodes();
+ assertEquals(3, actualKnownNodes.size());
+ assertEquals(expectedKnownNodes, actualKnownNodes);
+
+ cluster.stopJettySolrRunner(jettyNode1);
+ waitForCSPCacheTimeout();
+
+ actualKnownNodes = cspHttp.getKnownNodes();
+ assertEquals(3, actualKnownNodes.size());
+ assertEquals(expectedKnownNodes, actualKnownNodes);
+
+ cluster.stopJettySolrRunner(jettyNode3);
+ expectedKnownNodes = Set.of(jettyNode1Url, jettyNode2Url);
+ waitForCSPCacheTimeout();
+
+ // New nodes are removable from known hosts
+ actualKnownNodes = cspHttp.getKnownNodes();
+ assertEquals(2, actualKnownNodes.size());
+ assertEquals(expectedKnownNodes, actualKnownNodes);
+ }
+ }
+
+ private void waitForCSPCacheTimeout() throws InterruptedException {
+ Thread.sleep(6000);
Review Comment:
This test should set the system property `solr.solrj.cache.timeout.sec` to
maybe 1ms and then you can merely sleep for like a 100 milliseconds.
--
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]
