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-17533: Rearrange and cleanup solr.cmd #2822

Merged

Conversation

malliaridis
Copy link
Contributor

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

Description

By rearranging the code we can allow the java classes to handle more of the argument parsing and remove some more lines from bin/solr.cmd.

Solution

This PR rearranges the code in a way that it is a more "top-to-bottom" flow without many up-and-down jumps via goto. Additionally, it gets rid of unnecessary statements and parsing and forwards all interactions to SolrCLI if it is not one of the commands start, stop or restart. This approach is followed in bin/solr as well.

Tests

We are still lacking on tests for the bin/solr.cmd script. See SOLR-17508.

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 documentation for the Reference Guide

@@ -383,18 +355,19 @@ goto done
@echo.
goto done


REM Really basic command-line arg parsing
REM Parse arguments for special commands (start, stop, restart)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe say "auth" too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is skipped in case of the auth command. The auth command has its own parsing plus the java class parsing. If I am not mistaken, this was the same behavior before and if I am not mistaken bin/solr does the same. Correct me if I am wrong though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it does.. I think the way in bin/solr it is that first you do everything. Then you do auth. Then you do the remaining ones of start, stop, restart.

solr/bin/solr.cmd Outdated Show resolved Hide resolved
Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. One nit on the help args... So much cleaner. and I love that it ends up removing 298 lines of code!

@malliaridis malliaridis merged commit bc0c226 into apache:main Nov 9, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants