Skip to content

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Oct 6, 2024

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

Description

Please provide a short description of the changes you're making with this pull request.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

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

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 6, 2024
Snapshots added post moving to -url as a valid choice, so we need to add this back in.
Copy link
Contributor

@malliaridis malliaridis left a comment

Choose a reason for hiding this comment

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

Some hidden uses of -s that probably need to be updated as well:

  • solr/dev-tools/scripts/cloud.sh#337 in argsArray: -s should be replaced with --solr-home
  • solr/prometheus-exporter/bin/solr-exporter.cmd#81: -s should be replaced with --scrape-interval
  • solr/prometheus-exporter/bin/solr-exporter#115: -s should be replaced with --scrape-interval
  • solr/solr-ref-guide/modules/deployment-guide/pages/solr-control-script-reference.adoc
    • #214: example should use --solr-home
    • #281: -s should be --solr-home problably?
  • solr/solr-ref-guide/modules/deployment-guide/pages/enabling-ssl.adoc
    • #313: -s should be --solr-home
    • #322: -s should be --solr-home
  • solr/solr-ref-guide/modules/getting-started/pages/tutorial-films.adoc
    • #35: -s should be --solr-home
    • #43: -s should be --solr-home
  • solr/solr-ref-guide/modules/getting-started/pages/tutorial-solrcloud.adoc
    • #85: -s should be --solr-home
    • #179: -s should be --solr-home
    • #186: -s should be --solr-home
    • #199: -s should be --solr-home
    • #209: -s should be --solr-home
  • solr/solr-ref-guide/modules/getting-started/pages/tutorial-techproducts.adoc
    • #74: -s should be --solr-home
    • #81: -s should be --solr-home

Additional cases where we may consider replacing -s in the future:

  • solr/modules/hdfs/bin/prepare-snapshot-export.sh#46: The option -s is used to provide a source path
    we may consider not replacing it here, because the entrypoint is a different script

Did another check on -s uses
@epugh
Copy link
Contributor Author

epugh commented Oct 6, 2024

I really like how the --solr-home parameter looks in the docs. It's much less mysterious what is happening with the full parameter name used in the tutorials. We may want to use the long option in our tutorials a lot more...

Copy link
Contributor

@malliaridis malliaridis left a comment

Choose a reason for hiding this comment

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

On second review, I noticed multiple occurences in bin/solr.cmd (I searched for "-s"). I am not sure how -s behaves when used, because the script uses -s for solr-home and sorl-url in the same blocks. I will try to test it on a windows machine before approval.

Could you have a look as well @epugh? Nice catches by the way for the files I mentioned above, I saw that I forgot a few more there.

I also found for `-s` occurences in:

  • solr/solr-ref-guide/modules/getting-started/pages/tutorial-films.adoc#85 -s should be --shards
  • solr/solr-ref-guide/modules/deployment-guide/pages/solr-control-script-reference.adoc#108: -s should be --solr-home
  • solr/solr-ref-guide/modules/deployment-guide/pages/monitoring-with-prometheus-and-grafana.adoc#162: -s option should not be included for --scrape-interval

epugh and others added 3 commits October 6, 2024 13:48
…l-script-reference.adoc

Co-authored-by: Christos Malliaridis <c.malliaridis@gmail.com>
Co-authored-by: Christos Malliaridis <c.malliaridis@gmail.com>
Copy link
Contributor

@malliaridis malliaridis left a comment

Choose a reason for hiding this comment

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

Looks good and complete to me. :)

@epugh epugh merged commit 8d7edeb into apache:main Oct 8, 2024
4 of 5 checks passed
epugh added a commit that referenced this pull request Oct 8, 2024
The -s parameter now only means "provide a solr url" through out all of our most widely used Solr CLI scripts.

---------

Co-authored-by: Christos Malliaridis <c.malliaridis@gmail.com>
(cherry picked from commit 8d7edeb)
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