You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2023/02/06 20:14:27 UTC
[GitHub] [solr] epugh opened a new pull request, #1338: SOLR-16595: Standardize Solr Client Builders handling of times
epugh opened a new pull request, #1338:
URL: https://github.com/apache/solr/pull/1338
This change is going to touch a lot of places ;-)
https://issues.apache.org/jira/browse/SOLR-16595
# Description
Introduce the `TimeUnit unit` parameter to our Builder, and then internally convert what is passed in to what we need.
# Solution
Start converting builder methods.
# Tests
Run the existing test suite.
--
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] sonatype-lift[bot] commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104652457
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -454,7 +457,7 @@ public NamedList<Object> request(final SolrRequest<?> request, String collection
} else {
long currentStallTime =
TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - lastStallTime);
- if (currentStallTime > stallTime) {
+ if (currentStallTime > stallTimeMillis) {
Review Comment:
<picture><img alt="5% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/5/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Read/Write race. Non-private method `ConcurrentUpdateHttp2SolrClient.request(...)` reads without synchronization from `this.stallTimeMillis`. Potentially races with write in method `ConcurrentUpdateHttp2SolrClient.setPollQueueTime(...)`.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=388845913&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=388845913&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=388845913&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=388845913&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=388845913&lift_comment_rating=5)â]
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104733482
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -64,8 +64,8 @@ public class ConcurrentUpdateHttp2SolrClient extends SolrClient {
private boolean shutdownClient;
private boolean shutdownExecutor;
- private int pollQueueTime = 250;
Review Comment:
Humm.... Good darn question....
--
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] stillalex commented on pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1338:
URL: https://github.com/apache/solr/pull/1338#issuecomment-1428278393
did a pass through the changes (did not run the tests locally) and left a few minor comments. otherwise it looks good to me
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104747630
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -232,32 +232,32 @@ void evictStale() {
*/
@Deprecated
public void setRetryExpiryTime(int secs) {
- this.retryExpiryTime = TimeUnit.NANOSECONDS.convert(secs, TimeUnit.SECONDS);
+ this.retryExpiryTimeNano = TimeUnit.NANOSECONDS.convert(secs, TimeUnit.SECONDS);
}
protected final StateCache collectionStateCache = new StateCache();
class ExpiringCachedDocCollection {
final DocCollection cached;
- final long cachedAt;
+ final long cachedAtNano;
// This is the time at which the collection is retried and got the same old version
- volatile long retriedAt = -1;
+ volatile long retriedAtNano = -1;
// flag that suggests that this is potentially to be rechecked
volatile boolean maybeStale = false;
ExpiringCachedDocCollection(DocCollection cached) {
this.cached = cached;
- this.cachedAt = System.nanoTime();
+ this.cachedAtNano = System.nanoTime();
}
boolean isExpired(long timeToLiveMs) {
- return (System.nanoTime() - cachedAt)
+ return (System.nanoTime() - cachedAtNano)
Review Comment:
could we avoid the timeToLiveMs conversion? I see it hardcoded above as
```
protected volatile long timeToLiveMs = 60 * 1000L;
```
you could apply the same type of refactoring and have it nanos directly
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104741073
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -328,21 +329,22 @@ public Builder(Http2SolrClient http2Client, String... baseSolrUrls) {
* LBHttpSolrServer keeps pinging the dead servers at fixed interval to find if it is alive. Use
* this to set that interval
*
- * @param aliveCheckInterval time in milliseconds
+ * @param aliveCheckInterval how often to ping for aliveness
*/
- public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
+ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval, TimeUnit unit) {
if (aliveCheckInterval <= 0) {
throw new IllegalArgumentException(
"Alive check interval must be " + "positive, specified value = " + aliveCheckInterval);
}
- this.aliveCheckInterval = aliveCheckInterval;
+ this.aliveCheckIntervalMillis = TimeUnit.MILLISECONDS.convert(aliveCheckInterval, unit);
+ ;
Review Comment:
I think this ';' can be removed
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104444472
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -101,13 +116,27 @@ public B withConnectionTimeout(int connectionTimeoutMillis) {
* sockets.
*
* <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getSocketTimeout()}
+ *
+ * <p>* @deprecated Please use {@link #withSocketTimeout(long, TimeUnit)}
*/
+ @Deprecated(since = "9.2")
public B withSocketTimeout(int socketTimeoutMillis) {
+ withSocketTimeout(connectionTimeoutMillis, TimeUnit.MILLISECONDS);
Review Comment:
OMG... For some reason I was locked into looking for red underlines from intellij, and never even thought about this. In my other branch, I think I did the exact same thing again...
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1338:
URL: https://github.com/apache/solr/pull/1338#issuecomment-1428224962
Okay, I think I am finally done with the second part of this, which was to convert properties to having the units they are in as part of the property name. I suspect that we could continue this work for lots of other parts of the code base!
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104915599
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -454,7 +457,7 @@ public NamedList<Object> request(final SolrRequest<?> request, String collection
} else {
long currentStallTime =
TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - lastStallTime);
- if (currentStallTime > stallTime) {
+ if (currentStallTime > stallTimeMillis) {
Review Comment:
And this will go away eventually as setPollQueueTime is deprecated :-). We could make the field volatile but as long as we don't use it within Solr anywhere and it's deprecated, I think it's fine.
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104769574
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -64,8 +64,8 @@ public class ConcurrentUpdateHttp2SolrClient extends SolrClient {
private boolean shutdownClient;
private boolean shutdownExecutor;
- private int pollQueueTime = 250;
Review Comment:
IntelliJ told me the default is not actually ever used, it's always set.. so I removed it... However, maybe I made a mistake/ Thanks for looking!
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104727910
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -64,8 +64,8 @@ public class ConcurrentUpdateHttp2SolrClient extends SolrClient {
private boolean shutdownClient;
private boolean shutdownExecutor;
- private int pollQueueTime = 250;
Review Comment:
does this default value need to move to the builder? I'm not sure I am seeing it anywhere
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1105781681
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -232,32 +232,32 @@ void evictStale() {
*/
@Deprecated
public void setRetryExpiryTime(int secs) {
- this.retryExpiryTime = TimeUnit.NANOSECONDS.convert(secs, TimeUnit.SECONDS);
+ this.retryExpiryTimeNano = TimeUnit.NANOSECONDS.convert(secs, TimeUnit.SECONDS);
}
protected final StateCache collectionStateCache = new StateCache();
class ExpiringCachedDocCollection {
final DocCollection cached;
- final long cachedAt;
+ final long cachedAtNano;
// This is the time at which the collection is retried and got the same old version
- volatile long retriedAt = -1;
+ volatile long retriedAtNano = -1;
// flag that suggests that this is potentially to be rechecked
volatile boolean maybeStale = false;
ExpiringCachedDocCollection(DocCollection cached) {
this.cached = cached;
- this.cachedAt = System.nanoTime();
+ this.cachedAtNano = System.nanoTime();
}
boolean isExpired(long timeToLiveMs) {
- return (System.nanoTime() - cachedAt)
+ return (System.nanoTime() - cachedAtNano)
Review Comment:
i did a pass, but it looked a bit odd, and I'm worried about introducing a bug.. so I backed it out. But it does point to do we need a pass through Solr codebase and try and get all our time handling variables names and initialization patterns to 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 pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1338:
URL: https://github.com/apache/solr/pull/1338#issuecomment-1424332956
I broke the ConncurrentUpdateSolrClient unit test.. sigh.... First pass thorugh, no luck on why setting the timeout isn't working.
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104957402
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -151,9 +152,9 @@ public static class Builder {
protected Http2SolrClient.Builder internalClientBuilder;
private RequestWriter requestWriter;
private ResponseParser responseParser;
- private long retryExpiryTime =
+ private long retryExpiryTimeNano =
TimeUnit.NANOSECONDS.convert(3, TimeUnit.SECONDS); // 3 seconds or 3 million nanos
- private int timeToLiveSeconds = 60;
+ private long timeToLiveSeconds = TimeUnit.SECONDS.convert(60, TimeUnit.SECONDS);
Review Comment:
mostly to always use the same pattern... Though the more I look at it... The funnier maybe it is... Is there a pattern like "TimeUnit.SECONDS(60)"?
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1338:
URL: https://github.com/apache/solr/pull/1338#issuecomment-1421173580
I think that I got all the builders, but let me know if I missed one! Would love some eyes...
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1103966815
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -101,13 +116,27 @@ public B withConnectionTimeout(int connectionTimeoutMillis) {
* sockets.
*
* <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getSocketTimeout()}
+ *
+ * <p>* @deprecated Please use {@link #withSocketTimeout(long, TimeUnit)}
*/
+ @Deprecated(since = "9.2")
public B withSocketTimeout(int socketTimeoutMillis) {
+ withSocketTimeout(connectionTimeoutMillis, TimeUnit.MILLISECONDS);
Review Comment:
you aren't referring to the parameter; this is why the test failed. IntelliJ made this easy to spot; showed the parameter in gray.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -101,13 +116,27 @@ public B withConnectionTimeout(int connectionTimeoutMillis) {
* sockets.
*
* <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getSocketTimeout()}
+ *
+ * <p>* @deprecated Please use {@link #withSocketTimeout(long, TimeUnit)}
*/
+ @Deprecated(since = "9.2")
public B withSocketTimeout(int socketTimeoutMillis) {
+ withSocketTimeout(connectionTimeoutMillis, TimeUnit.MILLISECONDS);
+ return getThis();
+ }
+
+ /**
+ * Tells {@link Builder} that created clients should set the following read timeout on all
+ * sockets.
+ *
+ * <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getSocketTimeout()}
+ */
+ public B withSocketTimeout(long socketTimeout, TimeUnit unit) {
if (socketTimeoutMillis < 0) {
Review Comment:
again you aren't referring to the parameter
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -86,13 +87,27 @@ public B withFollowRedirects(boolean followRedirects) {
* Solr servers.
*
* <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getConnectTimeout()}
+ *
+ * @deprecated Please use {@link #withConnectionTimeout(long, TimeUnit)}
*/
+ @Deprecated(since = "9.2")
public B withConnectionTimeout(int connectionTimeoutMillis) {
+ withConnectionTimeout(connectionTimeoutMillis, TimeUnit.MILLISECONDS);
+ return getThis();
+ }
+
+ /**
+ * Tells {@link Builder} that created clients should obey the following timeout when connecting to
+ * Solr servers.
+ *
+ * <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getConnectTimeout()}
+ */
+ public B withConnectionTimeout(long connectionTimeout, TimeUnit unit) {
if (connectionTimeoutMillis < 0) {
Review Comment:
I suspect you had this problem because you renamed the parameter manually (as if your IDE was a simple text editor) to remove the "Millis" instead of telling your IDE (IntelliJ) to refactor-rename the parameter. By far this is one of the most common IDE functions I do -- rename. Memorize the hotkey. I think recent IntelliJ versions even have some sort of after-the-fact detection of this to do the rename if you simply edit but it's not as reliable.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -86,13 +87,27 @@ public B withFollowRedirects(boolean followRedirects) {
* Solr servers.
*
* <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getConnectTimeout()}
+ *
+ * @deprecated Please use {@link #withConnectionTimeout(long, TimeUnit)}
*/
+ @Deprecated(since = "9.2")
public B withConnectionTimeout(int connectionTimeoutMillis) {
+ withConnectionTimeout(connectionTimeoutMillis, TimeUnit.MILLISECONDS);
+ return getThis();
+ }
+
+ /**
+ * Tells {@link Builder} that created clients should obey the following timeout when connecting to
+ * Solr servers.
+ *
+ * <p>For valid values see {@link org.apache.http.client.config.RequestConfig#getConnectTimeout()}
+ */
+ public B withConnectionTimeout(long connectionTimeout, TimeUnit unit) {
if (connectionTimeoutMillis < 0) {
Review Comment:
you aren't referring to the parameter
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104966666
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -151,9 +152,9 @@ public static class Builder {
protected Http2SolrClient.Builder internalClientBuilder;
private RequestWriter requestWriter;
private ResponseParser responseParser;
- private long retryExpiryTime =
+ private long retryExpiryTimeNano =
TimeUnit.NANOSECONDS.convert(3, TimeUnit.SECONDS); // 3 seconds or 3 million nanos
- private int timeToLiveSeconds = 60;
+ private long timeToLiveSeconds = TimeUnit.SECONDS.convert(60, TimeUnit.SECONDS);
Review Comment:
It was fine the way it was; it reads in an obvious way.
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104916895
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -151,9 +152,9 @@ public static class Builder {
protected Http2SolrClient.Builder internalClientBuilder;
private RequestWriter requestWriter;
private ResponseParser responseParser;
- private long retryExpiryTime =
+ private long retryExpiryTimeNano =
TimeUnit.NANOSECONDS.convert(3, TimeUnit.SECONDS); // 3 seconds or 3 million nanos
- private int timeToLiveSeconds = 60;
+ private long timeToLiveSeconds = TimeUnit.SECONDS.convert(60, TimeUnit.SECONDS);
Review Comment:
LOL why?
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104747630
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -232,32 +232,32 @@ void evictStale() {
*/
@Deprecated
public void setRetryExpiryTime(int secs) {
- this.retryExpiryTime = TimeUnit.NANOSECONDS.convert(secs, TimeUnit.SECONDS);
+ this.retryExpiryTimeNano = TimeUnit.NANOSECONDS.convert(secs, TimeUnit.SECONDS);
}
protected final StateCache collectionStateCache = new StateCache();
class ExpiringCachedDocCollection {
final DocCollection cached;
- final long cachedAt;
+ final long cachedAtNano;
// This is the time at which the collection is retried and got the same old version
- volatile long retriedAt = -1;
+ volatile long retriedAtNano = -1;
// flag that suggests that this is potentially to be rechecked
volatile boolean maybeStale = false;
ExpiringCachedDocCollection(DocCollection cached) {
this.cached = cached;
- this.cachedAt = System.nanoTime();
+ this.cachedAtNano = System.nanoTime();
}
boolean isExpired(long timeToLiveMs) {
- return (System.nanoTime() - cachedAt)
+ return (System.nanoTime() - cachedAtNano)
Review Comment:
could we avoid the timeToLiveMs conversion to nanos? I see it hardcoded above as
```
protected volatile long timeToLiveMs = 60 * 1000L;
```
you could apply the same type of refactoring and have it nanos directly
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104752653
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java:
##########
@@ -80,8 +81,8 @@ public class LBHttpSolrClient extends LBSolrClient {
private final HttpSolrClient.Builder httpSolrClientBuilder;
private volatile Set<String> urlParamNames = new HashSet<>();
- private Integer connectionTimeout;
- private volatile Integer soTimeout;
+ private Long connectionTimeoutMillis;
+ private volatile Long soTimeoutMillis;
Review Comment:
this does not need to be volatile anymore. it only comes from the builder there are no setters. it is equivalent to connectionTimeoutMillis, they both have the same access pattern
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104752653
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java:
##########
@@ -80,8 +81,8 @@ public class LBHttpSolrClient extends LBSolrClient {
private final HttpSolrClient.Builder httpSolrClientBuilder;
private volatile Set<String> urlParamNames = new HashSet<>();
- private Integer connectionTimeout;
- private volatile Integer soTimeout;
+ private Long connectionTimeoutMillis;
+ private volatile Long soTimeoutMillis;
Review Comment:
this does not need to be volatile anymore. it only comes from the builder there are no setters.
--
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] stillalex commented on a diff in pull request #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104759729
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -64,8 +64,8 @@ public class ConcurrentUpdateHttp2SolrClient extends SolrClient {
private boolean shutdownClient;
private boolean shutdownExecutor;
- private int pollQueueTime = 250;
Review Comment:
sure. but I was asking about the default value. is that same as before? I'm not seeing it moved over.
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104746655
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -64,8 +64,8 @@ public class ConcurrentUpdateHttp2SolrClient extends SolrClient {
private boolean shutdownClient;
private boolean shutdownExecutor;
- private int pollQueueTime = 250;
Review Comment:
@stillalex I think that the method `public Builder setPollQueueTime(long pollQueueTime, TimeUnit unit)` sets `pollQueueTimeMillis` on the builder, which is then set as `this.pollQueueTimeMillis = builder.pollQueueTimeMillis;`
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1338:
URL: https://github.com/apache/solr/pull/1338
--
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 #1338: SOLR-16595: Standardize Solr Client Builders handling of times
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1338:
URL: https://github.com/apache/solr/pull/1338#discussion_r1104770485
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -328,21 +329,22 @@ public Builder(Http2SolrClient http2Client, String... baseSolrUrls) {
* LBHttpSolrServer keeps pinging the dead servers at fixed interval to find if it is alive. Use
* this to set that interval
*
- * @param aliveCheckInterval time in milliseconds
+ * @param aliveCheckInterval how often to ping for aliveness
*/
- public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
+ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval, TimeUnit unit) {
if (aliveCheckInterval <= 0) {
throw new IllegalArgumentException(
"Alive check interval must be " + "positive, specified value = " + aliveCheckInterval);
}
- this.aliveCheckInterval = aliveCheckInterval;
+ this.aliveCheckIntervalMillis = TimeUnit.MILLISECONDS.convert(aliveCheckInterval, unit);
+ ;
Review Comment:
thanks!
--
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