Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-16975: Remove loading of solr.xml from zookeeper (main branch only) #1920

Merged
merged 23 commits into from
Nov 11, 2023

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented Sep 13, 2023

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

Solr will now fail and exit if it finds /solr.xml in zookeeper, with a message on how to remove it.

Have also cleaned up docs and some scripts that used /solr.xml as example of how to upload stuff to zookeeper.

The MiniSolrCloudCluster relied on uploading a provided solr.xml to ZK, now it instead copies the provided file to each Jetty.

@janhoy janhoy marked this pull request as draft September 13, 2023 14:01
@janhoy janhoy marked this pull request as ready for review November 1, 2023 14:47
@janhoy janhoy marked this pull request as draft November 2, 2023 14:48

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite ZkFailoverTest to enforce a failed restart of cluster in a different way than relying on this sysprop hack...

@janhoy janhoy marked this pull request as ready for review November 7, 2023 20:26
@janhoy
Copy link
Contributor Author

janhoy commented Nov 7, 2023

So I fixed the last tests and broguht back some ZK connection code to NodeConfig. It really does not belong there, but ZkFailoverTest uses it simulate cluster restart failure by setting the sysprop waitForZk. It should be fixed but I don't want to tackle it here, so I added a TODO. Also opened https://issues.apache.org/jira/browse/SOLR-17071 to track the test improvement.

Final review welcome. @dsmiley ?

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Sad TODO there :-(

Copy link
Contributor

@HoustonPutman HoustonPutman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jan!

@janhoy janhoy merged commit 1e68438 into apache:main Nov 11, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants