Skip to content

SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods#1220

Merged
epugh merged 15 commits intoapache:mainfrom
epugh:SOLR-10463
Dec 22, 2022
Merged

SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods#1220
epugh merged 15 commits intoapache:mainfrom
epugh:SOLR-10463

Conversation

@epugh
Copy link
Contributor

@epugh epugh commented Dec 7, 2022

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

Description

Similar to a number of others, this moves the property to the Builder.

HOWEVER, one thing is that it appears in my searching the source that we do NOT use this property! I'd love to have someone confirm this.. Is there a chance this should actually be deprecated and removed????

Solution

did the migration, however maybe we rip it all out?

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:

  • 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

@epugh epugh requested review from dsmiley and risdenk December 7, 2022 13:14
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.

You added to the Builder but never consumed it! Do this in CloudSolrClient which is the base of the new & old impls.

You said we do not use this property but I believe you mean we do not set this property. It is used/retrieved in CloudSolrClient.

@epugh
Copy link
Contributor Author

epugh commented Dec 9, 2022

You added to the Builder but never consumed it! Do this in CloudSolrClient which is the base of the new & old impls.

You said we do not use this property but I believe you mean we do not set this property. It is used/retrieved in CloudSolrClient.

I was about to post a screenshot showing that this value was never used, and finally found it in a method shouldRetry. I duplicated the setting so if you use the builder you get it, if you don't you still get it. Maybe when we remove the deprecated methods, these defaults will just livei n the builder?

@epugh epugh requested a review from dsmiley December 15, 2022 15:50
@epugh
Copy link
Contributor Author

epugh commented Dec 15, 2022

@dsmiley @risdenk I think I am ready for another review after addressing feedback. Can I get a LGTM?

@epugh epugh merged commit e7cac79 into apache:main Dec 22, 2022
epugh added a commit that referenced this pull request Dec 22, 2022
…Client Builder methods (#1220)

Deprecated original method on clients, and added to builders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants