You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/12/07 13:14:32 UTC
[GitHub] [solr] epugh opened a new pull request, #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
epugh opened a new pull request, #1220:
URL: https://github.com/apache/solr/pull/1220
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](https://wiki.apache.org/solr/HowToContribute) 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](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) 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](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1049793352
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
we do have the "int secs" or "int connectionTimeoutMillis" as parameters on various methods. I wonder if we should have another JIRA to go through and make all timeunits on builders be the same????
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1055635045
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
I have added https://issues.apache.org/jira/browse/SOLR-16595 to do this over all builders...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1049791072
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
are you suggesting the method look like "public Builder setRetryExpiryTime(int secs, TimeUnit seconds)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1049801546
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -34,6 +35,8 @@
protected boolean useMultiPartPost;
protected Integer connectionTimeoutMillis = 15000;
protected Integer socketTimeoutMillis = 120000;
+ protected long retryExpiryTime =
Review Comment:
Good catch...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1049812581
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
@dsmiley I added https://issues.apache.org/jira/browse/SOLR-16590 to track standardizing across all builders the naming format of the methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1220:
URL: https://github.com/apache/solr/pull/1220#issuecomment-1344483969
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1049951107
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
TimeUnit class was introduced in part to add clarity to call-sites of a method so the unit is clear. `blah.setTime(TimeUnit.SECOND, 1)` is fine as well as `blah.setTime(TimeUnit.MINUTE,2)` -- the caller picks the unit convenient to them. With that design, the method is designed unit-free -- definitely NOT with variables named "second" as you proposed since the unit could be anything. Internally (implementation of the setter), we need to pick a unit to standardize to on some internal field to store the result, and name the field to be clear as to what the internal unit chosen is. (e.g. retryExpirySecs). Again, that's *internal*, the caller choses a unit convenient to them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1220:
URL: https://github.com/apache/solr/pull/1220#issuecomment-1353303993
@dsmiley @risdenk I think I am ready for another review after addressing feedback. Can I get a LGTM?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
epugh merged PR #1220:
URL: https://github.com/apache/solr/pull/1220
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1220: SOLR-10463: setRetryExpiryTime should be deprecated in favor of Solr Client Builder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1220:
URL: https://github.com/apache/solr/pull/1220#discussion_r1045179129
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -34,6 +35,8 @@
protected boolean useMultiPartPost;
protected Integer connectionTimeoutMillis = 15000;
protected Integer socketTimeoutMillis = 120000;
+ protected long retryExpiryTime =
Review Comment:
SolrClientBuilder is for all the Apache HttpClient based SolrClients, but retryExpiryTime is specifically for ZK. It's wrong to put here.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
Can you incorporate TimeUnit as a parameter so that it's clearer what the unit is at the call site?
IntelliJ tip: do this by selecting "TimeUnit.SECONDS" on the line below and doing cmd-opt-P (introduce parameter)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
+ */
+ public Builder setRetryExpiryTime(int secs) {
Review Comment:
No "with" terminology?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -245,6 +249,16 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /**
+ * This is the time to wait to refetch the state after getting the same state version from ZK
+ *
+ * <p>secs
Review Comment:
Delete, I think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org