Conversation
…when uploading a configset in bin/solr create. createCollection/createCore now just take a SolrClient; the only remaining ZK-specific step is resolving a live node's URL when only --zk-host is given. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Thanks for addressing this Eric :-)
| @@ -125,7 +127,7 @@ public Options getOptions() { | |||
| public void runImpl(CommandLine cli) throws Exception { | |||
| try (var solrClient = CLIUtils.getSolrClient(cli)) { | |||
There was a problem hiding this comment.
You could use CLIUtils.getCloudSolrClient(zkSolrConnection, builder) here...
| createCollection(cli); | ||
| createCollection(cli, solrClient); | ||
| } else { | ||
| createCore(cli, solrClient); |
There was a problem hiding this comment.
... and if the mode isn't actually cloud then at this line call cloudSolrClient.getHttpSolrClient().
| + cloudSolrClient.getClusterStateProvider().getQuorumHosts()); | ||
| // We will trust the config since we have the Zookeeper Address | ||
| configSetService.uploadConfig(confName, confPath); | ||
| + " using the Configsets V2 API"); |
There was a problem hiding this comment.
must we speak of the fact that we use that API specifically, especially "V2"?
There was a problem hiding this comment.
so, this only happens with verbose parameter, and I think it's a bit of debugging/helping someone understand which apis are being used... So yes, I think until we remove v1, someone might want to know "oh, it's using the v2 api"...
There was a problem hiding this comment.
Pull request overview
This PR updates bin/solr create to avoid direct ZooKeeper configset access by switching configset existence checks and uploads over to the Configsets V2 API, while also simplifying collection/core creation code paths to work with a generic SolrClient.
Changes:
- Replace direct ZooKeeper
/configs/<name>existence checks +ZkConfigSetServiceuploads with Configsets V2 APIListConfigSet+UploadConfigSet. - Refactor
createCollectionto acceptSolrClient(instead of building a dedicatedCloudSolrClientfrom--zk-host). - Add a new CLI test intended to cover the “upload new configset” path, plus a changelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| solr/core/src/java/org/apache/solr/cli/CreateTool.java | Switch configset existence + upload to Configsets V2 API and add local configset zipping helper for upload. |
| solr/core/src/test/org/apache/solr/cli/CreateToolTest.java | Add a test case for creating a collection that should upload a previously-nonexistent configset. |
| changelog/unreleased/SOLR-18320-skip-zk-for-configset-upload.yml | Document the behavioral change in bin/solr create regarding configset uploads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Okay, pending clean run of tests, this is done. |
| * | ||
| * @param rootPath the directory to zip | ||
| * @param validateFileTypes if true, a file with a forbidden extension (see {@link | ||
| * #isFileForbiddenInConfigSets}) causes an {@link IOException} instead of being silently |
There was a problem hiding this comment.
an IOException doesn't seem appropriate for such a problem
There was a problem hiding this comment.
I was hoping you'd put this in SolrJ somewhere. It would be useful for clients who want to submit zips! I can say it'd help me where I work, allowing us to remove some similar code.
|
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! |
Summary
bin/solr createpreviously uploaded configsets by opening a direct ZooKeeper connection (ZkConfigSetService) and reading/configs/<name>directly to check for an existing configset. It now does both via the Configsets V2 API (ConfigsetsApi.ListConfigSet/ConfigsetsApi.UploadConfigSet), zipping the local configset directory for upload.createCollection/createCorenow just take aSolrClientrather than requiring aCloudSolrClientbuilt from a ZooKeeper connection string. The only remaining ZK-specific step is resolving a live node's URL when the user only supplies--zk-host(no--solr-url) — done once viaCLIUtils.normalizeSolrUrl.Test plan
CreateToolTestpasses, including a newtestCreateCollectionUploadsNewConfigSettest that exercises the fresh-upload path (not just the reuse-existing-configset path the original test covered)./gradlew :solr:core:spotlessJavaCheckclean./gradlew :solr:core:compileJava :solr:core:compileTestJavaclean🤖 Generated with Claude Code