Skip to content

SOLR-17995: Deprecate "core URL" SolrStream constructors - #4830

Open
gerlowskija wants to merge 4 commits into
apache:mainfrom
gerlowskija:SOLR-17995-solrstream-coreUrl-removal
Open

SOLR-17995: Deprecate "core URL" SolrStream constructors#4830
gerlowskija wants to merge 4 commits into
apache:mainfrom
gerlowskija:SOLR-17995-solrstream-coreUrl-removal

Conversation

@gerlowskija

Copy link
Copy Markdown
Contributor

Description

Due to some recent additions, SolrStream currently has 4 different constructors in order to support taking in either "base" URLs or "core" URLs. This is a bit ugly.

Solution

This PR streamlines this by deprecating the "core URL" constructors and switching usages over to using the "base URL" constructors instead. The "core URLs" aren't removed entirely, though this can be done (on main only) in a subsequent PR.

Tests

Existing tests continue to 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, 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 a changelog entry for my change

Solr is moving towards using "base URLs" rather than "core URLs" where
possible. Standardizing on base URLs avoids baking v1 specifics into
many code-paths.  It also makes our code easier to understand:
historically we're not very good at documenting which URL pattern is
expected by which method, so standardizing brings a lot of readability
benefits.

This commit deprecates several SolrStream constructors that use "core
URLs".
@gerlowskija

Copy link
Copy Markdown
Contributor Author

FYI @dsmiley - I believe you asked about consolidating some of these ctors on a QT-removal PR...

@epugh epugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. It's slightly more verbose, but on the other hand, probably a lot better! I think we should have the since on the @Deprecated tag, similar to what is in #4750

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.

3 participants