[gerlowskija]

https://issues.apache.org/jira/browse/SOLR-18569

Description

Solr has long had code in ZkController to check whether the ephemeral /live_node entry from a previous process is still around, and wait for ZK to deleted it before proceeding.

But /live_node entries aren't the only ephemeral nodes that Solr expects to create on startup and that can linger after process exit. The /node_roles/ tree also contains ephemeral nodes. Currently, if any of these exist at startup-time, Solr will hit a NodeExists exception from ZK and fail to start up.

Solution

This commit changes ZkController's init code so that it now waits for these nodes as well. In practice, this prevents Solr from potentially stalling on start up because of an unexpected or unhandled NodeExists exception.

Tests

New test cases in ZkControllerTest to test how Solr handles these nodes existing when a new Solr node starts up. These tests aren't totally ideal as they involve a sleep or two: open to suggestions if anyone has ideas how to eliminate this.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added a changelog entry for my change

[nick-boss-tech]

🤖 AI text below 🤖 (reviewed by Nick Shanin)

@gerlowskija, here is a concrete version of a test suggestion for this test. It builds on the idea of checking the leftover entries right after the check returns, and it comes with a run on this change. It is based on the current head of this PR, 6f72e0cac151.

The problem. In testStartupWaitsForLeftoverEphemeralNodesToDisappear, the test joins the deleter thread in a finally block, and only after that asserts that the leftover paths are gone. If checkForExistingEphemeralNode() returned too early, the join would still wait for the deleter to remove the leftovers, and the assertion afterwards would pass. So the test cannot tell an early return from a correct one. This is about test sensitivity. I have not found a problem in the production code.

The change. The change is in solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java only. The absence assertion now runs right after checkForExistingEphemeralNode() returns, before the join. The join stays in finally, with a 30 second timeout so it cannot hang. The deleter thread's exception is captured and checked after the join, instead of being thrown inside the thread where the test would not see it. The test also checks that the deleter thread has finished. The production code is not touched.

Diff of this change only: nick-boss-tech@f00c7bd

Compare from the PR head to the new commit: nick-boss-tech/solr@6f72e0c...f00c7bd

What I ran. Each run was on Linux (aarch64) in a local copy, with the same temporary change where noted.

• At 6f72e0cac151 (the PR head), I temporarily emptied the body of checkForExistingEphemeralNode(), so it returns immediately. The test as it stands on the PR still passes. So it does not catch that early return.
• At f00c7bde83bf (the new commit), with the same temporary change, the updated test fails at its new assertion, with the message should be gone when the check returns for /node_roles/overseer/preferred/127.0.0.1:8983_solr. That is the failure the test should produce.
• At f00c7bde83bf, with no temporary change, ZkControllerTest runs 13 tests with 0 failures and 0 errors. Two tests are skipped by their existing @Ignore annotations: testVersionCompatibilityFailsStartup and testMinorVersionCompatibilityFailsStartup.

Limits. All of this ran on one Linux machine, not on Windows or in CI. The one second delay before the deleter removes the leftovers still provides the timing margin, so the test depends on that margin, just as the original did. A tighter handshake would need a point of coordination inside the production wait, and I did not add one, since that would mean changing production code for the test.

Would a test-only change like this be useful here, or would you rather keep the test as it is?

[gerlowskija]

Would a test-only change like this be useful here, or would you rather keep the test as it is?

Thanks @nick-boss-tech . I wasn't too worried about that case originally. The main indicator of checkForExistingEphemeralNodes success or failure is the exception thrown from that method if the nodes still exist at the end of the wait. But it's a nice little test hardening nonetheless. I've created a PR from your branch to bring the changes into this PR and merged it, thanks for the contribution! (gerlowskija#6)