[julianchandras]

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

Description

Since SOLR-17864, the default wait for ZooKeeper at node startup is 30000 seconds instead of 30. This is because when solr.cloud.wait.for.zk.seconds is not set, the fallback SolrZkClientTimeout.DEFAULT_ZK_CLIENT_TIMEOUT (already in milliseconds) is multiplied by 1000.

Also, the legacy system property -DwaitForZk is silently ignored, while the legacy env var SOLR_WAIT_FOR_ZK works with a deprecation warning.

Solution

• NodeConfig.getStartUpZkTimeOutMillis converts the property from seconds when it is set, and otherwise uses DEFAULT_ZK_CLIENT_TIMEOUT as is.
• The two legacy names canonicalize to different property names (SOLR_WAIT_FOR_ZK becomes solr.wait.for.zk, while -DwaitForZk becomes wait.for.zk), so a single entry in DeprecatedSystemPropertyMappings.properties can only match one of them. Modified EnvUtils to accept several comma-separated legacy names per property, and updated the entry to list both.

Written with Claude Code.

Tests

• TestSolrXml.java: Test that the solr.cloud.wait.for.zk.seconds property is read with the right unit
• EnvUtilsTest.java: Test that the legacy -DwaitForZk and SOLR_WAIT_FOR_ZK set solr.cloud.wait.for.zk.seconds and log a deprecation warning

TestSolrXml mostly covers solr.xml parsing, so I'm happy to move the new tests if there is a better location.

cc @epugh

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.

[github-actions]

Automated PR triage (LLM-generated; may be wrong). Checked once, when the PR was opened or marked ready; not updated on later pushes.

No clear mismatches in the answered judgements.

	Warranted?	In PR?	Confidence	Why
✅ Jira issue in title	—	yes	—	Already present; not judged.
✅ Changelog	fixed	fixed	high	The PR fixes a bug where the default ZooKeeper wait time was incorrectly calculated as 30000 seconds instead of 30 seconds, and restores functionality for the deprecated -DwaitForZk system property.​
✅ Reference Guide update	no	no	medium	The change fixes the behavior of the existing `solr.​cloud.​wait.​for.​zk.​seconds` property to match its documented intent (seconds) and restores a deprecated property; it does not introduce new configuration options or change documented behavior in a way that requires new documentation.​
✅ Upgrade notes	no	no	high	The change restores the intended default behavior (30 seconds) and fixes a deprecated property; it is not a backwards-incompatible change requiring user action.​
✅ Tests	yes	yes	high	The PR modifies production logic in `NodeConfig.​java` and `EnvUtils.​java` to fix unit conversion and legacy property mapping, which warrants new or updated tests to verify the correct behavior.​

⚠️ mismatch, ❔ uncertain or unanswered, ✅ agrees. Matching changelog, docs and test files were hidden from the model; the PR description may still mention them. File presence does not establish adequacy. Assessed commit ac2d1400646e366a3a79ae14b2d19b36d2d3d712.

[epugh]

in another PR that hasn't been merged i have the same thing!

[epugh]

nice!

[epugh]

okay, I thought everything was good (and the narrow fix is fine) but then see my nocommit in the code, we need to dig a bit more, we appear to have multiple proeprties that do the same thing???

@dsmiley you may want to weigh in here, I think we have a naming challenge of two properties that do the same thing? Or overlap in meaning? Please see my comment that I put into the code chagne.

[dsmiley]

What's the semantic difference question?

[dsmiley]

I'm going to be pedantic here: -DwaitForZk is java CLI flag that sets the waitForZk system property. It's not itself the system property. IMO just reference the latter; there are multiple ways to set a system property.

[dsmiley]

are we avoiding EnvUtils for some reason?

[epugh]

Good catch1

[epugh]

Any thoughts @dsmiley on the two different properties that maybe do the same thing? It appears that solr.cloud.wait.for.zk.seconds and solr.zookeeper.client.timeout overlap with each other?

[dsmiley]

Eric, this is another case of you (in #3312) publicizing (solr-properties.adoc) a very internal/test thing that shouldn't have been publicized. If you read this block of code, it begins with:

    // TODO: Only job of this block is to
    //  delay starting a solr core to satisfy
    //  ZkFailoverTest test case...

Which is itself sad... there's a fair amount of production code here in support of one test.

I don't see any context as to how the problem was discovered @julianchandras . Does this related to that test or perhaps simply found it via code inspection? Or, does this affect Solr in the real world? If the latter, then the comment is inaccurate as it apparently serves a production purpose. @janhoy added the comment in 2023. The underlying code to connect to ZK here was added by @gus-asf in #416 in 2021.

[julianchandras]

@dsmiley It wasn't a production incident. I'm currently working on a research tool that looks at past fixes and checks whether they still hold in the current Solr version.

I found SOLR-18630 while checking against SOLR-14503, which reported an actual faulty behaviour of Solr in cloud mode. In this case, what I found is that the default wait is 30000s instead of 30s, and that the waitForZk system property isn't respected.

I'm unsure about the TODO comment, but waitForZk came from user reports (SOLR-5129, SOLR-14503) and sets the connection timeout during startup, so I do think it matters outside the test