Feature/solr-17334 Minor bugs in Solr dedicated coordinator mode

[ellaeln]

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

Description

Addressing a few minor bugs in solr's coordinator behavior. These issues are:

• It's impossible to request solr's root source on a coordinator node

• Coordinator requests are enabled for the /select handler only

• From outside proxied and coordinator requests cannot be distinguished making it hard to debug requests

Solution

• adding an if to catch cases i which the collection is null
• removing the line that required the use of /select
• adding a node tracking property that would appear in debugQuery mode

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:

• [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• [x] I have created a Jira issue and added the issue ID to my pull request title.
• [x] 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)
• [x] I have developed this patch against the main branch.
• [x] I have run ./gradlew check.
• [ ] I have added tests for my changes.
• [ ] I have added documentation for the Reference Guide

[ellaeln]

opened a new MR for solr/17334 to expedite merging these changes @cpoerschke please let me know if there's anything a can do to assist moving forward

[ellaeln]

Thanks for the review :) Is there anything we can do to expedite the merge? @cpoerschke @noblepaul

[cpoerschke]

Hello. Next steps here could be to:

1. review and address the 'gradle precommit' failure above
2. review the suggestions (and apply them if they are agreeable)

[ellaeln]

Hello. Next steps here could be to:

1. review and address the 'gradle precommit' failure above
2. review the suggestions (and apply them if they are agreeable)

I investigated the Gradle precommit failure and found that the issue was due to a missing empty line at the end of the file. I’ve added the line and tested it locally, so it should be resolved now.

Also I viewed your suggestions and added them to the PR, I thank you for raising them

[cpoerschke]

Thanks @ellaeln for iterating on this PR, the CI is now green, yay!

w.r.t. the remainder of the review(s) here -- unfortunately I'm not familiar enough with the area of the code to help review that, but hopefully others will be.

(Solr 9.7 was released earlier this month, so my guesstimate would be that maybe in November or December there might be a Solr 9.8 release.)

So if (perhaps) there is no further review input here before then, my suggestion would be to reach out on the dev list in a couple of weeks i.e. early October. Hope that helps.

[mkhludnev]

I think it's fine to add CHANGES row and push.

[github-actions[bot]]

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

[dsmiley]

I'm looking at #2527 and #2672 (this one) which seem the same at a glance; why do we have two PRs?

It's really sad this didn't get merged before 9.8. Contributors sometime really need to advocate / push for their changes or they get forgotten if no committer is interested.

@patsonluk maybe you could take a look at this PR as you have worked on the Coordinator node and run it production (I assume)

Another vote of confidence to this PR (actually #2527 if that matters) is that a colleague took it today in order to try out coordinator node.

[patsonluk]

Sorry about the delay! I was on vacation and somehow I missed this thread!

To be honest I'm not totally sure why it returns null for non-select paths:

if (!path.endsWith("/select")) return null;

My guess is that the design is to avoid all non query operations on Coordinator node as those might not be supported (so to avoid any surprises). Perhaps @noblepaul could shed some light here? ✨

As for the 2 others changes, they LGTM! 😊

• adding an if to catch cases i which the collection is null
• adding a node tracking property that would appear in debugQuery mode

[mkhludnev]

Doesn't it make sense to add a test confirming this functionality?

[github-actions[bot]]

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

[CommRogue]

Any update on this? @patsonluk

It would be very nice to see this merged before 10.

Also, there still seems to be two PRs opened for this - this one and #2527.

[github-actions[bot]]

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

[github-actions[bot]]

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.