SOLR-10470: setParallelCacheRefreshes should be deprecated in favor of SolrClientBuilder methods #1248

epugh · 2022-12-22T23:42:30Z

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

Description

setParallelCacheRefreshes should be deprecated in favor of SolrClientBuilder methods

Solution

Please provide a short description of the approach taken to implement your solution.

epugh · 2022-12-23T11:51:02Z

So, with these changes, a lot of tests fail... For example "DistributedExpandComponentTest"

epugh · 2022-12-23T16:47:30Z

So... I can't get a clean run of tests on any branch of solr on my laptop....

epugh · 2022-12-23T16:48:42Z

So, setParallelCacheRefreshes appears to be a odd setting.... Does anyone actually change it/use it? Or do we all just use the default of 3? There don't appear to be any tests for it.... Thoughts on eliminating it?

dsmiley

Thoughts on eliminating it?

I think it's preferable to make tuning settings settable instead of being hard-coded. That said, maybe it could be a global setting with a system property if it's expert enough. I'm +1 to doing that. Maybe solr.cloudSolrClient.parallelCacheRefreshLocks

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java

dsmiley · 2022-12-23T17:54:16Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

+  /**
+   * If caches are expired they are refreshed after acquiring a lock. This method sets the number of
+   * locks. It is used by the Builder only.
+   */
+  void setParallelCacheRefreshesLocks(int parallelCacheRefreshesLocks) {
+    locks = objectList(parallelCacheRefreshesLocks);
+  }
+


I don't see this used by the builder, I see it used by the constructor. I think we don't need this method; it could be inline'ed.

was looking for a place to put this observation, so I'll leave it here. I think I would prefer having a method that takes an 'int' and hides away the implementation. this would allow in the future for replacing this cache striping with another cache that also allows for setting the stripe size without breaking any clients that rely on this (if ever).

I think I would prefer having a method that takes an 'int' and hides away the implementation.

That exists in this PR -- Builder.setParallelCacheRefreshes. This makes the method I'm commenting on,
Builder.setParallelCacheRefreshesLocks more dubious as to if it's needed/useful.

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

stillalex · 2022-12-25T17:23:21Z

left a few minor comments, overall this looks good.

is updating the objectList method on the table? I find this code a bit ugly (bigger change would be to use an array everywhere, list is not doing any favors. smaller change would be to at least return List instead of ArrayList)

epugh · 2022-12-27T23:45:29Z

left a few minor comments, overall this looks good.

is updating the objectList method on the table? I find this code a bit ugly (bigger change would be to use an array everywhere, list is not doing any favors. smaller change would be to at least return List instead of ArrayList)

There are a lot of places where some clean up would be nice. I think we should add a new JIRA for these clean ups. I think it's an opportunity to see if there are any otehr java cleanups that could be done.

…p2SolrClient.java Co-authored-by: David Smiley <dsmiley@apache.org>

epugh · 2022-12-27T23:58:17Z

I think I've responded to everythign from @dsmiley and @stillalex.. Only change I haven't made is to eliminate this property and replace it with a system variable... That feels like a bit more work, cause then I have to add more docs to ref guide etc... I can live iwth just moving it.... Thoughts? Should we not move forward with this PR and just add the system propertly?

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java

stillalex · 2022-12-28T16:03:39Z

I agree eliminating the property is more work. I don't have a preference here. I would add the system property support (left a comment at the relevant line) and not remove builder support yet. basically allow both options, just my 2 cents.

dsmiley · 2022-12-28T21:24:46Z

I don't think we need to document expert/advanced things in the reference guide like this. I'm not saying there shouldn't be any such there, only that we should not be compelled to.

epugh · 2022-12-29T12:26:02Z

left a few minor comments, overall this looks good.
is updating the objectList method on the table? I find this code a bit ugly (bigger change would be to use an array everywhere, list is not doing any favors. smaller change would be to at least return List instead of ArrayList)

There are a lot of places where some clean up would be nice. I think we should add a new JIRA for these clean ups. I think it's an opportunity to see if there are any otehr java cleanups that could be done.

@stillalex I created https://issues.apache.org/jira/browse/SOLR-16600 to track your idea, love a PR!

epugh · 2022-12-29T12:29:21Z

I just fixed the lint problem. While it would be good to think about what properties should be settable and which should be system properties, I actually am thinking that is out of scope. I'd rather do that as a seperate PR, and where we think about ALL the properties and if they are system property settable.. (I also want to get Solr to a better way of handling property setting globablly anyway... ). So if you'all can live with it, I'd like to merge this as is.... @stillalex I added a JIRA for the simplification idea you had...

epugh · 2022-12-30T12:16:35Z

Okay, any angst to me merging this?

stillalex · 2022-12-30T23:06:17Z

looks good to me, will leave @dsmiley to add any concerns if any.

dsmiley

Okay with me to delay System property use. But I'm dubious of a big grand over-haul all at once happening. Incremental progress vs grand over-haul.

FYI there's an interesting JIRA https://issues.apache.org/jira/browse/SOLR-15960 somewhat related and a colleague has been working on it slowly.

dsmiley · 2022-12-30T22:18:40Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

+  /**
+   * If caches are expired they are refreshed after acquiring a lock. This method sets the number of
+   * locks. It is used by the Builder only.
+   */
+  void setParallelCacheRefreshesLocks(int parallelCacheRefreshesLocks) {
+    locks = objectList(parallelCacheRefreshesLocks);
+  }
+


I think I would prefer having a method that takes an 'int' and hides away the implementation.

That exists in this PR -- Builder.setParallelCacheRefreshes. This makes the method I'm commenting on,
Builder.setParallelCacheRefreshesLocks more dubious as to if it's needed/useful.

Avoid solving naming confusion by just inlining the method in the one place we use it!

epugh · 2022-12-31T13:55:33Z

thanks for the comments folks. Running a full test, and waiting for the jenkins main branch to finish, and then will merge this.

…f SolrClientBuilder methods (#1248) Introduce Builder setter for parallelCacheRefreshes on cloud SolrClients. Deprecated direct setter setParallelCacheRefreshes on cloud SolrClients. Inlined logic for creating object locks.

migrate to builder...

6fd17d8

epugh requested a review from dsmiley December 23, 2022 16:47

dsmiley reviewed Dec 23, 2022

View reviewed changes

stillalex reviewed Dec 25, 2022

View reviewed changes

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java Show resolved Hide resolved

epugh and others added 4 commits December 27, 2022 18:46

Merge remote-tracking branch 'upstream/main' into SOLR-10470

ce0eb86

Update solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHtt…

b3242cc

…p2SolrClient.java Co-authored-by: David Smiley <dsmiley@apache.org>

respond to feedback

8efaebc

respond to feedback

d44601e

stillalex reviewed Dec 28, 2022

View reviewed changes

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java Show resolved Hide resolved

tidy

dd60601

dsmiley reviewed Dec 30, 2022

View reviewed changes

dsmiley approved these changes Dec 30, 2022

View reviewed changes

inline the method, reducing duplicate naming confusion.

3c3d421

Avoid solving naming confusion by just inlining the method in the one place we use it!

epugh added 2 commits December 31, 2022 17:09

track changes

fc688c7

Merge branch 'main' into SOLR-10470

bb98313

epugh merged commit ac57877 into apache:main Dec 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-10470: setParallelCacheRefreshes should be deprecated in favor of SolrClientBuilder methods #1248

SOLR-10470: setParallelCacheRefreshes should be deprecated in favor of SolrClientBuilder methods #1248

epugh commented Dec 22, 2022

epugh commented Dec 23, 2022

epugh commented Dec 23, 2022

epugh commented Dec 23, 2022

dsmiley left a comment

dsmiley Dec 23, 2022

stillalex Dec 25, 2022

dsmiley Dec 30, 2022

stillalex commented Dec 25, 2022

epugh commented Dec 27, 2022

epugh commented Dec 27, 2022

stillalex commented Dec 28, 2022

dsmiley commented Dec 28, 2022

epugh commented Dec 29, 2022

epugh commented Dec 29, 2022

epugh commented Dec 30, 2022

stillalex commented Dec 30, 2022

dsmiley left a comment

dsmiley Dec 30, 2022

epugh commented Dec 31, 2022

SOLR-10470: setParallelCacheRefreshes should be deprecated in favor of SolrClientBuilder methods #1248

SOLR-10470: setParallelCacheRefreshes should be deprecated in favor of SolrClientBuilder methods #1248

Conversation

epugh commented Dec 22, 2022

Description

Solution

epugh commented Dec 23, 2022

epugh commented Dec 23, 2022

epugh commented Dec 23, 2022

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley Dec 23, 2022

Choose a reason for hiding this comment

stillalex Dec 25, 2022

Choose a reason for hiding this comment

dsmiley Dec 30, 2022

Choose a reason for hiding this comment

stillalex commented Dec 25, 2022

epugh commented Dec 27, 2022

epugh commented Dec 27, 2022

stillalex commented Dec 28, 2022

dsmiley commented Dec 28, 2022

epugh commented Dec 29, 2022

epugh commented Dec 29, 2022

epugh commented Dec 30, 2022

stillalex commented Dec 30, 2022

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley Dec 30, 2022

Choose a reason for hiding this comment

epugh commented Dec 31, 2022