bin/solr in branch_9_7 has commands that fail #2710

epugh · 2024-09-13T15:24:11Z

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

Description

Fixes some bugs that existing in branch_9_7.

-p port numbers
bin/solr healthcheck is broken

This has good stuff to port back to main and branch_9x...

Tests

New Bats tests added

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 · 2024-09-13T15:37:01Z

Look at that! in 9.6, -V is passed in as verbose to the CreateTool. https://github.com/apache/solr/blob/branch_9_6/solr/bin/solr#L1185

This is generating some other refactorings which will be fun to bring to main and 9x seperately!

epugh · 2024-09-13T16:17:51Z

Found that HealthcheckTool isn't working in 9.7 either.. and our Bats test doesn't refute the right "ERROR", only "error" ;-).

…he other or we default to localhost.

epugh · 2024-09-13T20:37:35Z

Okay, I think this is ready for review and commit. I am thinking that I move the bats tests in another PR to branch_9x to see what breaks there, and then do a PR to fix branch_9x. Then, I take the good stuff here and apply it to main... Versus doing it the other way...

janhoy

Thanks for quick PR. Looking good, not taken it for a spin, but very nice with the BATS tests.

Will we need edit to solr.cmd you think?

janhoy · 2024-09-14T15:36:46Z

solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java

-          try (var solrClientForCollection = SolrCLI.getSolrClient(coreUrl)) {
+
+          // SolrCLI.getSolrClient converts the coreUrl back into a root Solr url, but we need to
+          // talk to specific core so manually creating the client.


Hmm, would you say this is a bug, or is it well documented in the javadoc?

Humm..... I read it and it says:

/** * Get a SolrClient for the given URL. The URL can be with or without /solr suffix, the method * will normalize it and add '/solr'. To support certain unit tests, the hostContext can be * overridden via the system property 'hostContext'. * * <p>Note that from Solr 10.0 Solr will use / as base URL to support /api and other contexts, so * handling of hostContext differs between Solr 9.x and 10.x. * * @param solrUrl the base URL of Solr * @return a SolrClient initialized with correct hostContext, default '/solr' */ public static SolrClient getSolrClient(String solrUrl) {

I think this is the only place we need a core specific url... Do you think it should support that use case as well?

I looked a bit more at the code, and decided to go with the javadoc approach!

solr/core/src/java/org/apache/solr/cli/ToolBase.java

janhoy · 2024-09-14T15:44:48Z

solr/packaging/test/test_create.bats

+  solr assert --started http://localhost:${SOLR_PORT} --timeout 5000
+
+  run solr create -c COLL_NAME -V
+  assert_output --partial "Created new core 'COLL_NAME'"


Should not this assert a string that is only printed in verbose mode?

Good question... I think for core, you got this message, and for collection, you got a different dump (that was more like the api response). I changed it so that both core and collection output a short message like this, and then if you add the -v then you ALSO see the api responses... Does this work for you.

Sure, I just expected the assert to fail without the -V, which it won't now since the text asserted is always printed?

I finally understand what you mean, and am looking at it and then will merge.

solr/packaging/test/test_delete_collection.bats

solr/packaging/test/test_healthcheck.bats

I didn't try to have it be smart, like "Oh, you are using it with a core and throw an error"

epugh · 2024-09-18T16:27:12Z

Thanks for quick PR. Looking good, not taken it for a spin, but very nice with the BATS tests.

Will we need edit to solr.cmd you think?

Checked and yes, we had to fix healthcheck in solr.cmd.

epugh · 2024-09-18T16:28:04Z

@janhoy I've gone through and addressed (hopefully!) all your commits. Can I get a LGTM?

malliaridis

A few small additions from my side.

malliaridis · 2024-09-28T15:23:56Z

solr/core/src/java/org/apache/solr/cli/CreateTool.java

+        SolrCLI.OPTION_ZKHOST_DEPRECATED,
+        SolrCLI.OPTION_VERBOSE_DEPRECATED);


Should this be eventually

Suggested change

SolrCLI.OPTION_ZKHOST_DEPRECATED,

SolrCLI.OPTION_VERBOSE_DEPRECATED);

SolrCLI.OPTION_ZKHOST_DEPRECATED,

SolrCLI.OPTION_VERBOSE,

SolrCLI.OPTION_VERBOSE_DEPRECATED);

malliaridis · 2024-09-28T15:25:15Z

solr/core/src/java/org/apache/solr/cli/DeleteTool.java

+        SolrCLI.OPTION_ZKHOST_DEPRECATED,
+        SolrCLI.OPTION_VERBOSE_DEPRECATED);


Same here:

Suggested change

SolrCLI.OPTION_ZKHOST_DEPRECATED,

SolrCLI.OPTION_VERBOSE_DEPRECATED);

SolrCLI.OPTION_ZKHOST_DEPRECATED,

SolrCLI.OPTION_VERBOSE,

SolrCLI.OPTION_VERBOSE_DEPRECATED);

solr/packaging/test/test_create.bats

gus-asf · 2024-09-28T18:27:47Z

Can we update the title of this? is it just about CLI stuff? hard to know what this is about...

Co-authored-by: Christos Malliaridis <[email protected]>

epugh · 2024-09-28T20:56:38Z

This was committed into branch_9x in #35ae5f7c55c27f463db048b975d06ec070fbbeef

Re-introduce support for -p and -port

b0da37e

Now supporting the deprecated -V (versus -v)

2b18333

This is generating some other refactorings which will be fun to bring to main and 9x seperately!

epugh added 5 commits September 13, 2024 12:43

fix a bug in HealthcheckTool itself and how we pass parameters in

5c0d208

whitespace

777aefd

restor the -p and -port to bin/solr delete

872d79e

nicer messaging

49ffeb4

make sure -V for verbose still works.

bf4c495

epugh requested a review from janhoy September 13, 2024 18:20

epugh changed the title ~~Re-introduce support for -p and -port~~ Re-introduce support for -p and -port in bin/solr create and bin/solr delete. Fix bin/solr healthcheck. Sep 13, 2024

epugh added 2 commits September 13, 2024 14:52

we don't force a check for -z or --solr-url, but you do need one or t…

3ae097d

…he other or we default to localhost.

maybe have the magic incantation?

b1bb0ab

epugh changed the title ~~Re-introduce support for -p and -port in bin/solr create and bin/solr delete. Fix bin/solr healthcheck.~~ Fix bugs in bin/solr in branch_9_7 Sep 13, 2024

janhoy reviewed Sep 14, 2024

View reviewed changes

epugh added 4 commits September 18, 2024 11:16

remove dead code

7a5a51f

Use the params that exist in 9.7

74f66d8

Provide some docs why you wouldn't want to use this method with a core.

394a7b1

I didn't try to have it be smart, like "Oh, you are using it with a core and throw an error"

No need for return.

ad7ebb8

Nicer messaging

e8ee34b

janhoy approved these changes Sep 18, 2024

View reviewed changes

improve test to check verbose mode

edbff4b

malliaridis requested changes Sep 28, 2024

View reviewed changes

epugh changed the title ~~Fix bugs in bin/solr in branch_9_7~~ bin/solr in branch_9_7 has commands that fail Sep 28, 2024

Update solr/packaging/test/test_create.bats

f90ce33

Co-authored-by: Christos Malliaridis <[email protected]>

epugh merged commit 21a6b4c into apache:branch_9_7 Sep 28, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bin/solr in branch_9_7 has commands that fail #2710

bin/solr in branch_9_7 has commands that fail #2710

epugh commented Sep 13, 2024 •

edited

Loading

epugh commented Sep 13, 2024

epugh commented Sep 13, 2024

epugh commented Sep 13, 2024

janhoy left a comment

janhoy Sep 14, 2024

epugh Sep 18, 2024

epugh Sep 18, 2024

janhoy Sep 14, 2024

epugh Sep 18, 2024

janhoy Sep 18, 2024

epugh Sep 24, 2024

epugh commented Sep 18, 2024

epugh commented Sep 18, 2024

malliaridis left a comment

malliaridis Sep 28, 2024

malliaridis Sep 28, 2024 •

edited

Loading

gus-asf commented Sep 28, 2024

epugh commented Sep 28, 2024

		SolrCLI.OPTION_ZKHOST_DEPRECATED,
		SolrCLI.OPTION_VERBOSE_DEPRECATED);

bin/solr in branch_9_7 has commands that fail #2710

bin/solr in branch_9_7 has commands that fail #2710

Conversation

epugh commented Sep 13, 2024 • edited Loading

Description

Tests

Checklist

epugh commented Sep 13, 2024

epugh commented Sep 13, 2024

epugh commented Sep 13, 2024

janhoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented Sep 18, 2024

epugh commented Sep 18, 2024

malliaridis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malliaridis Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

gus-asf commented Sep 28, 2024

epugh commented Sep 28, 2024

epugh commented Sep 13, 2024 •

edited

Loading

malliaridis Sep 28, 2024 •

edited

Loading