Re: [PR] SOLR-18005: ConcurrentUpdateJettySolrClient [solr]

["gerlowskija (via GitHub)" <gi...@apache.org>]

gerlowskija commented on code in PR #3885:
URL: https://github.com/apache/solr/pull/3885#discussion_r2578230720


##########
solr/solr-ref-guide/modules/deployment-guide/pages/solrj.adoc:
##########
@@ -147,7 +147,7 @@ include::example$UsingSolrJRefGuideExamplesTest.java[tag=solrj-solrclient-timeou
 
 When these values are not explicitly provided, SolrJ falls back to using the defaults for the OS/environment is running on.
 
-`ConcurrentUpdateSolrClient` and its counterpart `ConcurrentUpdateHttp2SolrClient` also implement a stall prevention
+`ConcurrentUpdateSolrClient` and its counterpart `ConcurrentUpdateBaseSolrClient` also implement a stall prevention

Review Comment:
   [-1] Why are we referencing these two particular classes here?  The first is the deprecated Apache version of the client, and the second is an abstract base class that users are unlikely to ever care about.  Neither seems as relevant as the "Concurrent" client that we actually want folks to use: ConcurrentUpdateJettySolrClient.
   
   Wouldn't it make more sense to only reference "ConcurrentUpdateJettySolrClient"?  That'd keep this section more in line with the "Types of SolrClients" section above.  Or optionally, reference "ConcurrentUpdateSolrClient" and "ConcurrentUpdateJettySolrClient" in keeping with the intent of this line prior to this PR?
   
   



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -381,112 +373,10 @@ public void setAuthenticationStore(AuthenticationStore authenticationStore) {
   }
 
   /** (visible for testing) */
-  public long getIdleTimeout() {
+  public long getIdleTimeoutMillis() {

Review Comment:
   [-0] I much prefer the new name you've chosen here, don't get me wrong.  But it is a breaking change in a very public and user-facing SolrJ class, so it might be nice to mention it in a changelog entry to provide context for users who stumble across the change upon upgrading to SolrJ 10



-- 
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