Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from Dec 31, 2022
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6fd17d8
migrate to builder...
epugh Dec 22, 2022
ce0eb86
Merge remote-tracking branch 'upstream/main' into SOLR-10470
epugh Dec 27, 2022
b3242cc
Update solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHtt…
epugh Dec 27, 2022
8efaebc
respond to feedback
epugh Dec 27, 2022
d44601e
respond to feedback
epugh Dec 27, 2022
dd60601
tidy
epugh Dec 29, 2022
3c3d421
inline the method, reducing duplicate naming confusion.
epugh Dec 31, 2022
fc688c7
track changes
epugh Dec 31, 2022
bb98313
Merge branch 'main' into SOLR-10470
epugh Dec 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
View file
Open in desktop
Expand Up @@ -97,6 +97,7 @@ protected CloudHttp2SolrClient(Builder builder) {
} else {
this.stateProvider = builder.stateProvider;
}
this.setParallelCacheRefreshesLocks(builder.parallelCacheRefreshesLocks);
dsmiley marked this conversation as resolved.
Show resolved Hide resolved

this.lbClient = new LBHttp2SolrClient.Builder(myClient).build();
}
Expand Down Expand Up @@ -147,6 +148,7 @@ public static class Builder {
private ResponseParser responseParser;
private long retryExpiryTime =
TimeUnit.NANOSECONDS.convert(3, TimeUnit.SECONDS); // 3 seconds or 3 million nanos
private int parallelCacheRefreshesLocks = 3;
dsmiley marked this conversation as resolved.
Show resolved Hide resolved

/**
* Provide a series of Solr URLs to be used when configuring {@link CloudHttp2SolrClient}
Expand Down Expand Up @@ -249,6 +251,17 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}

/**
* When caches are expired then they are refreshed after acquiring a lock. Use this to set the
* number of locks.
*
* <p>Defaults to 3.
*/
public Builder setParallelCacheRefreshes(int parallelCacheRefreshesLocks) {
this.parallelCacheRefreshesLocks = parallelCacheRefreshesLocks;
return this;
}

/**
* This is the time to wait to refetch the state after getting the same state version from ZK
*/
Expand Down
11 changes: 11 additions & 0 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java
View file
Open in desktop
Expand Up @@ -1222,11 +1222,22 @@ public boolean isDirectUpdatesToLeadersOnly() {
/**
* If caches are expired they are refreshed after acquiring a lock. use this to set the number of
* locks
*
* @deprecated use {@link CloudHttp2SolrClient.Builder#setParallelCacheRefreshes(int)} instead
*/
@Deprecated
public void setParallelCacheRefreshes(int n) {
locks = objectList(n);
}

/**
* 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);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

protected static ArrayList<Object> objectList(int n) {
ArrayList<Object> l = new ArrayList<>(n);
for (int i = 0; i < n; i++) l.add(new Object());
Expand Down