Skip to content

Comments

SOLR-15421 ConfigSetService.checkConfigExists accepts empty configs#135

Merged
dsmiley merged 1 commit intoapache:mainfrom
asalamon74:SOLR-15421
Jun 1, 2021
Merged

SOLR-15421 ConfigSetService.checkConfigExists accepts empty configs#135
dsmiley merged 1 commit intoapache:mainfrom
asalamon74:SOLR-15421

Conversation

@asalamon74
Copy link
Contributor

Description

Both FileSystemConfigSetService and ZkConfigSetService only checks the existence of the directory only in checkConfigExists.

Solution

Create a new method to also check for solrconfig.xml checkConfigExistsWithSolrConfigXml

Tests

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)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a existsConfig == null check above, there should be one here as well, otherwise there will be a NPE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing something; why two calls to ZK just to check that a config exists? If solrconfig.xml is now necessary, then why check the parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fxied

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing something; why two calls to ZK just to check that a config exists? If solrconfig.xml is now necessary, then why check the parent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the isDirectory check; why not just check for solrconfig.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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.

LGTM. Can you add a CHANGES.txt entry please? Under 9.0 Improvements. Try to explain briefly in a way a user would understand.

@asalamon74
Copy link
Contributor Author

Thanks @dsmiley I added the CHANGES.txt entry and also changed the commit message to match it. Feel free to suggest a new text if necessary.

@dsmiley dsmiley merged commit f3b4693 into apache:main Jun 1, 2021
epugh pushed a commit to epugh/solr that referenced this pull request Oct 22, 2021
…#135)

ConfigSet API now checks for solrconfig.xml when checking for the existence of a configset
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.

3 participants