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.

Sign up for GitHub

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

Jump to bottom

SOLR-17382: remove -a and use full name for addlopts --additional-options #2684

Merged
merged 7 commits into from
Sep 10, 2024
Merged

SOLR-17382: remove -a and use full name for addlopts --additional-options #2684

merged 7 commits into from
Sep 10, 2024

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Aug 30, 2024

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

Description

Fix up the start command docs, and deprecate "addlopts" in RunExampleTool

Solution

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

Tests

none :-(

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

@epugh
Copy link
Contributor Author

epugh commented Aug 30, 2024

Okay, I think this is ready!

janhoy
janhoy reviewed Sep 4, 2024
View reviewed changes
Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

I'm ok with deprecating -a. But --additiona-options is quite verbose. Would it work with --jvm-opts being both shorter and clearer?

@epugh
Copy link
Contributor Author

epugh commented Sep 4, 2024

I'm ok with deprecating -a. But --additiona-options is quite verbose. Would it work with --jvm-opts being both shorter and clearer?

I like --jvm-opts... --jvm-options ? Or is that just feeling overly verbose. I looked to see if either options or opts was reused elsewhere in the CLI and didn't see one to guide us one way or the other! GIve me a vote and we can move forward.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 6, 2024
@epugh
Copy link
Contributor Author

epugh commented Sep 6, 2024

I took your suggestion @janhoy. The -a remains for back compat in the 9x. Can I get a LGTM?

@epugh epugh merged commit 53c0c3c into apache:main Sep 10, 2024
5 checks passed
epugh added a commit that referenced this pull request Sep 11, 2024
@epugh
SOLR-17382: remove -a and use full name for addlopts --additional-opt…
4f834d6 
…ions (#2684)

* move -a and -addlopts to deprecated and use --jvm-opts for long format.
* this option is only called by scripts, its not called by end users, so we don't need deprecation in RunExampleTool

(cherry picked from commit 53c0c3c)
malliaridis
malliaridis reviewed Sep 11, 2024
View reviewed changes
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.

I left a few comments for potential breaking changes in case of backporting. Besides that, I'm fine too with jvm-opts as a replacement. :)

solr/bin/solr.cmd
@@ -426,7 +426,7 @@ IF "%1"=="-s" goto set_solr_url
IF "%1"=="--solr-url" goto set_solr_url
IF "%1"=="-solrUrl" goto set_solr_url
IF "%1"=="-a" goto set_addl_opts
IF "%1"=="--additional-options" goto set_addl_opts
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we have to keep this for backwards compatibility in branch_9x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we decided to go with --jvm-opts instead of ```-addopts` this was never released so we are okay.

solr/core/src/java/org/apache/solr/cli/RunExampleTool.java
Comment on lines -184 to -185
Option.builder("a")
.argName("OPTS")
Copy link
Contributor

Choose a reason for hiding this comment

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

By removing this option should the DefaultParser not throw an UnrecognizedOptionException if the option is not present during parsing but the user passes -a to the CLI?

This would break backwards compatibility as well if we plan to backport the changes to 9X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your question now.. So the thing that is unqiue about RunExampleTool compared to our other Java *Tool is that the arguments passed to it are NOT the ones typed by the human user! Instead, the start command parses all the args and creates a NEW set of commands to RunExampleTool. So we can change how RunExampleTool works as LONG as we maps the args in the start command.

Copy link
Contributor

@malliaridis malliaridis Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That makes sense. I was just unsure how changes in public classes should be treated in general, so I assumed it would also require deprecation for cases where someone accesses and uses the class(es) directly.

Thanks for clarifying. :)

solr/packaging/test/test_ssl.bats
solr assert --started https://localhost:${SOLR_PORT} --timeout 5000

# server2 will run on $SOLR2_PORT and will use server2.keystore. Initially, this is the same as server1.keystore
export SOLR_SSL_KEY_STORE=$ssl_dir/server2.keystore.p12
export SOLR_SSL_TRUST_STORE=$ssl_dir/server2.keystore.p12

# leaving -a instead of --jvm-opts for back compat testing.
solr start -c -z localhost:${ZK_PORT} -p ${SOLR2_PORT} -a "-Dsolr.jetty.sslContext.reload.scanInterval=1 -DsocketTimeout=5000"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should fail based on my observations on a sample project. Can someone confirm that this works correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to double check now by hand on main and branch_9x. I believe it will work because we left that -a in the parsing logic for the start command...

@epugh
Copy link
Contributor Author

epugh commented Sep 12, 2024

Thanks for going through and commenting @malliaridis, I just did some manual testing with 9x and it all looks okay!

I ran:

bin/solr start -a "-Dsolr.jetty.sslContext.reload.scanInterval=1 -DsocketTimeout=5000"

And saw the new params in the solr admin ui list of args!

@malliaridis
Copy link
Contributor

Thanks for the replies and for double-checking everything for me. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malliaridis malliaridis malliaridis left review comments

@janhoy janhoy janhoy approved these changes

Assignees
No one assigned
Labels
cat:cli documentation Improvements or additions to documentation start-scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@epugh @malliaridis @janhoy