dsmiley commented on code in PR #3305:
URL: https://github.com/apache/solr/pull/3305#discussion_r2027934645
##########
solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateReaderTest.java:
##########
@@ -894,68 +894,57 @@ public void testDeletePrsCollection() throws Exception {
}
}
- public void testGetLowestSolrVersion() throws Exception {
+ /**
+ * Test that when two live nodes have valid SemVer strings,
fetchLowestSolrVersion() returns the
+ * numerically lower version.
+ */
+ public void testFetchLowestSolrVersion_validNodes() throws Exception {
SolrZkClient zkClient = fixture.zkClient;
ZkStateReader reader = fixture.reader;
- String livePath = ZkStateReader.LIVE_NODES_ZKNODE; // usually "/live_nodes"
+ String livePath = ZkStateReader.LIVE_NODES_ZKNODE;
- // Remove any existing live node children.
+ // Clear any existing live node children.
List<String> nodes = zkClient.getChildren(livePath, null, true);
for (String node : nodes) {
zkClient.delete(livePath + "/" + node, -1, true);
}
+ // Create two live nodes with valid SemVer strings.
String node1 = "node1_solr";
Map<String, Object> props1 = new HashMap<>();
- props1.put(LIVE_NODE_SOLR_VERSION, "9.1.0");
+ props1.put(LIVE_NODE_SOLR_VERSION, "9.1.2");
props1.put(LIVE_NODE_NODE_NAME, node1);
byte[] data1 = Utils.toJSON(props1);
zkClient.create(livePath + "/" + node1, data1, CreateMode.EPHEMERAL, true);
String node2 = "node2_solr";
Map<String, Object> props2 = new HashMap<>();
- props2.put(LIVE_NODE_SOLR_VERSION, "8.0.3.3.1");
+ props2.put(LIVE_NODE_SOLR_VERSION, "9.0.1");
props2.put(LIVE_NODE_NODE_NAME, node2);
byte[] data2 = Utils.toJSON(props2);
zkClient.create(livePath + "/" + node2, data2, CreateMode.EPHEMERAL, true);
- String node3 = "node3_solr";
- Map<String, Object> props3 = new HashMap<>();
- props3.put(LIVE_NODE_SOLR_VERSION, "9.1.2");
- props3.put(LIVE_NODE_NODE_NAME, node3);
- byte[] data3 = Utils.toJSON(props3);
- zkClient.create(livePath + "/" + node3, data3, CreateMode.EPHEMERAL, true);
-
- String lowestVersion = reader.getLowestSolrVersion();
- assertEquals("Check lowest version", "8.0.3.3.1", lowestVersion);
+ String lowestVersion = reader.fetchLowestSolrVersion();
+ assertEquals("Expected lowest version to be 9.0.1", "9.0.1",
lowestVersion);
}
- public void testGetLowestSolrVersionMalformedVersion() throws Exception {
+ /** Test that when the only live node has empty data,
fetchLowestSolrVersion() returns null. */
+ public void testFetchLowestSolrVersion_noData() throws Exception {
Review Comment:
this only tests when one node is empty and none have a version. I don't see
a test for both (one blank, one non-blank).
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -874,14 +875,12 @@ public void removeLiveNodesListener(LiveNodesListener
listener) {
/**
* Returns the lowest Solr version among all live nodes in the cluster.
*
- * @return the lowest Solr version as a String, or null if no version is
found
- * @throws KeeperException if a ZooKeeper error occurs
- * @throws InterruptedException if the thread is interrupted
+ * @return the lowest Solr version as a String, or null if no version is
found, throw Semver with
+ * invalid version
Review Comment:
```suggestion
* @return the lowest Solr version as a String, possibly null if
<em>any</em> node doesn't have the version.
```
-- it's important to work this way because the caller needs to know if the
whole cluster is at least a certain version.
What's unclear is what to do if there are no live nodes. Null wouldn't make
sense (as it would indicate an old Solr is running); LATEST (current software)
would make sense. Okay but now imagine that there are some live nodes of Solr
10.2 while the current code is 10.0 but has no live node. It would probably
make sense to consider the current code as the lowest version, even if it has
no live node registered. It's an edge case.
In essence, we're looking for a "smallestSolrVersionSeen", to include
ourselves, looking at live nodes as well. Null means older than 9.9 which is
when this will be released.
--
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
