SOLR-18086: Fix LBSolrClient behavior in case of zombie nodes (#4130)

Author: anshumg

changelog/unreleased/SOLR-18086.yml

@@ -0,0 +1,9 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Stale EndpointIterator state causes unexpected order when there are zombie servers in LBSolrClient
+type: fixed # added, changed, fixed, deprecated, removed, dependency_update, security, other
+authors:
+  - name: Chris Hostetter
+  - name: Anshum Gupta
+links:
+  - name: SOLR-18086
+    url: https://issues.apache.org/jira/browse/SOLR-18086

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java

@@ -383,6 +383,7 @@ private void fetchNext() {
               skipped.add(wrapper.getEndpoint());
             }
           }
+          endpoint = null;
           continue;
         }
 

solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientTest.java

@@ -96,20 +96,53 @@ public void testServerIterator() throws SolrServerException {
   public void testServerIteratorWithZombieServers() throws SolrServerException {
     HashMap<String, LBSolrClient.EndpointWrapper> zombieServers = new HashMap<>();
     LBSolrClient.Req req = new LBSolrClient.Req(new QueryRequest(), SOLR_ENDPOINTS);
-    LBSolrClient.EndpointIterator endpointIterator =
+
+    // pick a random number of endpoints to mark as zombies
+    final int numToShuffle = random().nextInt(SOLR_ENDPOINTS.size());
+    // yes, this might pick the same endpoint multiple times; fine for our purposes
+    for (int i = 0; i < numToShuffle; i++) {
+      final LBSolrClient.Endpoint z = SOLR_ENDPOINTS.get(random().nextInt(SOLR_ENDPOINTS.size()));
+      zombieServers.put(z.getUrl(), new LBSolrClient.EndpointWrapper(z));
+    }
+    // Try those on the Zombie list after all other possibilities are exhausted.
+    final List<LBSolrClient.Endpoint> expectedOrder = new ArrayList<>(SOLR_ENDPOINTS);
+    for (LBSolrClient.Endpoint e : SOLR_ENDPOINTS) {
+      if (zombieServers.containsKey(e.getUrl())) {
+        expectedOrder.remove(e);
+        expectedOrder.add(e);
+      }
+    }
+
+    final LBSolrClient.EndpointIterator endpointIterator =
         new LBSolrClient.EndpointIterator(req, zombieServers);
-    zombieServers.put("2", new LBSolrClient.EndpointWrapper(new LBSolrClient.Endpoint("2")));
+    final List<LBSolrClient.Endpoint> actualOrder = new ArrayList<>();
+    while (endpointIterator.hasNext()) {
+      actualOrder.add(endpointIterator.nextOrError());
+    }
+    assertFalse(endpointIterator.hasNext()); // sanity check double call
+    assertEquals("randomZombies(" + zombieServers.keySet() + ")", expectedOrder, actualOrder);
+    LuceneTestCase.expectThrows(SolrServerException.class, endpointIterator::nextOrError);
+  }
 
-    assertTrue(endpointIterator.hasNext());
-    assertEquals(new LBSolrClient.Endpoint("1"), endpointIterator.nextOrError());
-    assertTrue(endpointIterator.hasNext());
-    assertEquals(new LBSolrClient.Endpoint("3"), endpointIterator.nextOrError());
-    assertTrue(endpointIterator.hasNext());
-    assertEquals(new LBSolrClient.Endpoint("4"), endpointIterator.nextOrError());
+  @Test
+  public void testServerIteratorWithAllZombies() throws SolrServerException {
+    HashMap<String, LBSolrClient.EndpointWrapper> zombieServers = new HashMap<>();
+    LBSolrClient.Req req = new LBSolrClient.Req(new QueryRequest(), SOLR_ENDPOINTS);
+    for (LBSolrClient.Endpoint e : SOLR_ENDPOINTS) {
+      zombieServers.put(e.getBaseUrl(), new LBSolrClient.EndpointWrapper(e));
+    }
 
-    // Try those on the Zombie list after all other possibilities are exhausted.
-    assertTrue(endpointIterator.hasNext());
-    assertEquals(new LBSolrClient.Endpoint("2"), endpointIterator.nextOrError());
+    final LBSolrClient.EndpointIterator endpointIterator =
+        new LBSolrClient.EndpointIterator(req, zombieServers);
+    // if everyone is a zombie, then the original sorted preference order of
+    // endpoints should still be respected
+    final List<LBSolrClient.Endpoint> actualOrder = new ArrayList<>();
+    while (endpointIterator.hasNext()) {
+      actualOrder.add(endpointIterator.nextOrError());
+    }
+    assertFalse(endpointIterator.hasNext()); // sanity check double call
+    assertEquals(SOLR_ENDPOINTS, actualOrder);
+    LuceneTestCase.expectThrows(SolrServerException.class, endpointIterator::nextOrError);
   }
 
   @Test