-
Notifications
You must be signed in to change notification settings - Fork 679
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.
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
SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics #2899
Changes from 1 commit
5261cd3
bc8c888
a249169
1f8a307
90bf9e2
51676f5
db76064
605bdc7
508af0d
39177b4
35651b8
19577dc
ca59536
67c8b8f
7b90862
1539d97
8f17279
effedb9
29a3445
50470cd
f423bd2
6176a99
783f815
e8c998d
e035a0c
99282aa
45b9e10
7df3c3b
90bd2ec
f984d12
28aec3b
30dcdb3
730aad0
071fb04
05eb1ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…-built clients
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,7 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory | |
protected ExecutorService commExecutor; | ||
|
||
protected volatile Http2SolrClient defaultClient; | ||
protected Http2SolrClient.Builder httpClientBuilder; | ||
protected InstrumentedHttpListenerFactory httpListenerFactory; | ||
protected LBHttp2SolrClient<Http2SolrClient.Builder> loadbalancer; | ||
|
||
|
@@ -305,16 +306,16 @@ public void init(PluginInfo info) { | |
sb); | ||
int soTimeout = | ||
getParameter(args, HttpClientUtil.PROP_SO_TIMEOUT, HttpClientUtil.DEFAULT_SO_TIMEOUT, sb); | ||
var defaultClientBuilder = | ||
this.httpClientBuilder = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please name |
||
new Http2SolrClient.Builder() | ||
.withConnectionTimeout(connectionTimeout, TimeUnit.MILLISECONDS) | ||
.withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS) | ||
.withExecutor(commExecutor) | ||
.withMaxConnectionsPerHost(maxConnectionsPerHost) | ||
.withListenerFactory(List.of(this.httpListenerFactory)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed an issue. This overwrites the list of listeners that may already be present. Shouldn't we be augmenting, not replacing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to work within the bounds of the existing API, but I agree this should be an add, not a replace. I modified the API, and broke backwards-compatibility so as to not silently introduce bugs in users' applications. |
||
this.defaultClient = defaultClientBuilder.build(); | ||
this.defaultClient = httpClientBuilder.build(); | ||
|
||
this.loadbalancer = new LBHttp2SolrClient.Builder<>(defaultClientBuilder).build(); | ||
this.loadbalancer = new LBHttp2SolrClient.Builder<>(httpClientBuilder).build(); | ||
|
||
initReplicaListTransformers(getParameter(args, "replicaRouting", null, sb)); | ||
|
||
|
@@ -323,8 +324,10 @@ public void init(PluginInfo info) { | |
|
||
@Override | ||
public void setSecurityBuilder(HttpClientBuilderPlugin clientBuilderPlugin) { | ||
System.out.println("IN SET SEC BUILDER"); | ||
new Exception().printStackTrace(System.out); | ||
if (clientBuilderPlugin != null) { | ||
clientBuilderPlugin.setup(defaultClient); | ||
clientBuilderPlugin.setup(httpClientBuilder, defaultClient); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,4 +34,8 @@ public interface HttpClientBuilderPlugin { | |
public SolrHttpClientBuilder getHttpClientBuilder(SolrHttpClientBuilder builder); | ||
|
||
public default void setup(Http2SolrClient client) {} | ||
|
||
public default void setup(Http2SolrClient.Builder httpClientBuilder, Http2SolrClient client) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe these should be split up; setup a client and builder separately (2 methods)? Feels weird to take both in setup(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree separate methods would be nicer. But the implementing classes do things on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you point me at such so I can appreciate the problem better? |
||
setup(client); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -374,8 +374,17 @@ PublicKey fetchPublicKeyFromRemote(String nodename) { | |
} | ||
} | ||
|
||
@Override | ||
public void setup(Http2SolrClient.Builder httpClientBuilder, Http2SolrClient client) { | ||
setup(client, httpClientBuilder); | ||
} | ||
|
||
@Override | ||
public void setup(Http2SolrClient client) { | ||
setup(client, null); | ||
} | ||
|
||
private void setup(Http2SolrClient client, Http2SolrClient.Builder builder) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why have this separate and with different parameter order? In other words, lets make this one the |
||
final HttpListenerFactory.RequestResponseListener listener = | ||
new HttpListenerFactory.RequestResponseListener() { | ||
private static final String CACHED_REQUEST_USER_KEY = "cachedRequestUser"; | ||
|
@@ -431,7 +440,12 @@ private Optional<String> getUserFromJettyRequest(Request request) { | |
(String) request.getAttributes().get(CACHED_REQUEST_USER_KEY)); | ||
} | ||
}; | ||
client.addListenerFactory(() -> listener); | ||
if(client != null) { | ||
client.addListenerFactory(() -> listener); | ||
} | ||
if(builder != null) { | ||
builder.withListenerFactory(List.of(() -> listener)); | ||
} | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT @iamsanjay ?