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-17285: Move RemoteSolrException to SolrClient in v10 #2587

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

samuelrivascoding
Copy link
Contributor

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

Description

Moved RemoteSolrException to SolrClient from BaseHttpSolrClient

Solution

Moved RemoteSolrException class to SolrClient. Manually changed import statements from BaseHttpSolrClient to SolrClient if is not needed anymore for RemoteSolrException. Changed BaseHttpSolrClient.RemoteSolrException to SolrClient.RemoteSolrException

Tests

OUTPUT-org.apache.solr.cloud.HttpPartitionOnCommitTest.txt
OUTPUT-org.apache.solr.cloud.ReplaceNodeTest.txt
OUTPUT-org.apache.solr.handler.TestSolrConfigHandlerConcurrent.txt
OUTPUT-org.apache.solr.pkg.TestPackages.txt

I ran ./gradlew dev. It worked fine
./gradlew check did not work. It said there were failing tests on unrelated files

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.

Thanks!
It's remarkable how many files were touched by this; I wouldn't have guessed.
Just need a CHANGES.txt entry in "Other" under the Solr 10 section.
like "SOLR-17285: RemoteSolrException moved from BaseHttpSolrClient to SolrClient (your name here)"

@dsmiley
Copy link
Contributor

dsmiley commented Jul 23, 2024

I am looking at BaseHttpSolrClient again, and I see that RemoteExecutionException is also something that'll need to be moved. It's not used in many places. I think it's reasonable to include that in the scope of this JIRA issue and PR; WDYT? But I don't love the name "RemoteExecutionException" and I'm not even convinced it needs to exist; like couldn't RemoteSolrException take a payload? Publishing it to SolrClient would give it undeserved visibility IMO. Hmmm. On the other hand, based on how it's used, this is so internal that maybe it should simply move to HttpSolrClientBase (which is the successor to BaseHttpSolrClient). I welcome your thoughts.

Copy link

This PR has had no activity for 60 days and is now labeled as stale. Any new activity or converting it to draft 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. Thank you for your contribution!

@github-actions github-actions bot added the stale PR not updated in 60 days label Sep 22, 2024
@dsmiley
Copy link
Contributor

dsmiley commented Sep 24, 2024

@samuelrivascoding how shall I credit you (name you) in CHANGES.txt? If I don't hear back, it'll be literally that.

@github-actions github-actions bot removed the stale PR not updated in 60 days label Sep 26, 2024
@dsmiley
Copy link
Contributor

dsmiley commented Sep 28, 2024

Will merge as soon as Monday. I also plan to handle RemoteSolrException in a follow-up.

@dsmiley dsmiley merged commit bd4d55c into apache:main Oct 2, 2024
4 checks passed
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