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-17153: Resolve IndexOutOfBoundsException in HttpSolrCall.resolveDocCollection() #2392

Closed
wants to merge 0 commits into from

Conversation

aparnasuresh85
Copy link
Contributor

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

Description

Resolve an IndexOutOfBoundsException in HttpSolrCall.resolveDocCollection(), at line:
String collectionName = collectionsList.get(0);

Solution

Check if collectionsList is empty, in that case return early.

Tests

Ensured existing tests pass

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)
  • 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

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

+1 to the change.
needs "tidy" clearly (you don't have a space to the left of the parenthesis)

Also something was eating at it me a bit... the method you added, resolveDocCollection, returns a DocCollection but in HttpSolrCall.init you added a call to it that does nothing with the response. Feels weird. Instead, shouldn't HttpSolrCall.getCoreByCollection use this method?

@dsmiley
Copy link
Contributor

dsmiley commented Apr 7, 2024

This PR looks weird; looks like the old PR but one commit added. Should be a new PR.

@aparnasuresh85
Copy link
Contributor Author

aparnasuresh85 commented Apr 7, 2024

+1 to the change. needs "tidy" clearly (you don't have a space to the left of the parenthesis)

Also something was eating at it me a bit... the method you added, resolveDocCollection, returns a DocCollection but in HttpSolrCall.init you added a call to it that does nothing with the response. Feels weird. Instead, shouldn't HttpSolrCall.getCoreByCollection use this method?

I agree. The reason I kept the call to resolveDocCollection() outside of getCoreByCollection() is because the doc around the latter (if getCoreByCollection returned null ) said "// this collection exists , but this node does not have a replica for that collection".

Instead, should we throw a SolrException (collection or alias not found) if inside getCoreByCollection, the call to resolveDocCollection() returned null?

@aparnasuresh85
Copy link
Contributor Author

+1 to the change. needs "tidy" clearly (you don't have a space to the left of the parenthesis)
Also something was eating at it me a bit... the method you added, resolveDocCollection, returns a DocCollection but in HttpSolrCall.init you added a call to it that does nothing with the response. Feels weird. Instead, shouldn't HttpSolrCall.getCoreByCollection use this method?

I agree. The reason I kept the call to resolveDocCollection() outside of getCoreByCollection() is because the doc around the latter (if getCoreByCollection returned null ) said "// this collection exists , but this node does not have a replica for that collection".

Instead, should we throw a SolrException (collection or alias not found) if inside getCoreByCollection, the call to resolveDocCollection() returned null?

Maybe not. I see a number of tests fail if an exception is thrown in case collection is null from resolveDocCollection(). The tests pass otherwise. I will send a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants