You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2025/03/02 23:07:34 UTC
[PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
dsmiley opened a new pull request, #3233:
URL: https://github.com/apache/solr/pull/3233
and remove HttpListenerFactory. HTTP request listeners live in HttpClient now, not Http2SolrClient. Lifecycle is different. Expose HttpClient from Http2SolrClient.
https://issues.apache.org/jira/browse/SOLR-17688
From JIRA:
> The Http2SolrClient.Builder holds a list of HttpListenerFactory, a custom Solr thing with a get method called per-request, returning something extending the inner class RequestResponseListener, that in turn implements 3 Jetty client callback methods. It's cleaner/simpler to replace those custom Solr abstractions with simply a list of Jetty Request.Listener, and that which is registered directly on the Jetty HttpClient. This means less tracking work for Http2SolrClient as it no longer holds the list of these directly and arranges to call them. It also means the HttpClient can be re-used with metrics, tracing, authentication for passing to Jetty's ProxyServlet – [SOLR-17286](https://issues.apache.org/jira/browse/SOLR-17286). But it does change the listener lifecycle to be per-client instead of per-request, which impacts one of our two implementations.
10.0 only.
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-2719102856
I've hit a blocking test failure in which https://issues.apache.org/jira/browse/SOLR-13510 has reappeared here because onQueued isn't necessarily called from the submitting thread :-( `org.apache.solr.security.BasicAuthOnSingleNodeTest#basicTest` It failed on me once; hasn't repeated. Even though I can guess I'll need to maybe call onQueued or similar, I want to deterministically induce the situation so it can better be understood and more consistently captured in a regression.
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-3064321539
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active 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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-2714527929
interesting change, I think it looks good and the new version of the code looks much cleaner, +1 from me.
just 2 thoughts (feel free to ignore if not relevant):
- there are some places where we create new clients based on existing clients settings, so would we need to make sure the listeners are being copied over? (not sure if this is a problem here, I remember running into this pattern a while back).
- removing a solr abstraction in favor of a jetty abstraction brings the code closer to jetty clients by creating this hard dependency. and while I have not been around enought to tell, it feels like long term it would be better to depend less on specific http clients, giving the project some freedom in picking a new impl if needed (would this work with the new jdk http client?).
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-2715838346
Thanks for your feedback Alex!
> there are some places where we create new clients based on existing clients settings, so would we need to make sure the listeners are being copied over? (not sure if this is a problem here, I remember running into this pattern a while back).
I *think* this will be less of an issue now because the HttpClient (thus everything attached to it) goes along for the ride. This PR shows that we no longer need to copy them when the httpClient is provided in the builder. But I suppose that if a user creates an HttpClient on their own for whatever reason, then the onus is on them to configure it with whatever listeners are desired; previously this was separate. But that's true with all the other settings in the builder associated with the HttpClient construction (e.g. everything createHttpClient looks at in the builder). It underscores a point that if you provide an HttpClient to the builder, it's an expert use-case; you know what you are doing because we don't document every last setting, wether it applies or not if someone provides the client.
> removing a solr abstraction in favor of a jetty abstraction brings the code closer to jetty clients by creating this hard dependency. and while I have not been around enought to tell, it feels like long term it would be better to depend less on specific http clients, giving the project some freedom in picking a new impl if needed (would this work with the new jdk http client?).
I've only seen one switch in the project's history (Apache -> Jetty) so I don't think it's worth insulating the differences. New abstractions are not free; abstractions should be documented as well; don't benefit from knowledge the user might already have in Jetty and they can get in the way. I'd rather see Solr reduce code wherever it can; removing frivolous abstractions is one. Solr's listener factory thing being removed here doesn't apply to the HttpJdkSolrClient (or else you'd have seen the impact in your review).
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-2870297986
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3233:
URL: https://github.com/apache/solr/pull/3233#discussion_r1976742439
##########
solr/core/src/java/org/apache/solr/util/stats/InstrumentedHttpListenerFactory.java:
##########
@@ -86,38 +87,44 @@ private static String methodNameString(Request request) {
}
@Override
- public RequestResponseListener get() {
- return new RequestResponseListener() {
- Timer.Context timerContext;
- Span span = Span.getInvalid();
-
- @Override
- public void onQueued(Request request) {
- // do tracing onQueued because it's called from Solr's thread
- span = Span.current();
- TraceUtils.injectTraceContext(request);
- }
+ public final void onQueued(Request req) {
+ var listener = new PerRequestListener(req);
Review Comment:
this scenario is maybe a bit more awkward because on onQueued, we need to register a listener specific for this request because the listener has per-request state (fields). This was previously happening for all listeners in Http2SolrClient but you'll see it gets removed there now.
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -377,8 +376,8 @@ PublicKey fetchPublicKeyFromRemote(String nodename) {
@Override
public void setup(Http2SolrClient client) {
- final HttpListenerFactory.RequestResponseListener listener =
- new HttpListenerFactory.RequestResponseListener() {
+ final var listener =
Review Comment:
this listener no per-request state so can simply add this listener to client level.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -632,12 +613,6 @@ private void decorateRequest(Request req, SolrRequest<?> solrRequest, boolean is
}
setBasicAuthHeader(solrRequest, req);
- for (HttpListenerFactory factory : listenerFactory) {
Review Comment:
No longer need to do this; the listeners are now at httpClient level, not request level. Granted the listener onQueued can choose to register another listener (and one does do this).
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/PreemptiveBasicAuthClientBuilderFactory.java:
##########
@@ -95,10 +95,9 @@ public void setup(Http2SolrClient client, String basicAuthUser, String basicAuth
authenticationStore.addAuthentication(
new SolrBasicAuthentication(basicAuthUser, basicAuthPass));
client.setAuthenticationStore(authenticationStore);
- client.getProtocolHandlers().put(new WWWAuthenticationProtocolHandler(client.getHttpClient()));
Review Comment:
because I removed client.getProtocolHandlers() now that HttpClient is exposed
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-2707447434
Planning to merge Tuesday. Beforehand will look closer at the errors that I did see, which seemed intermittent and unrelated.
CHANGES.txt comment proposal in Other:
> SolrJ: replaced Http2SolrClient.Builder.withListenerFactory with withRequestListeners; using Jetty abstractions instead, and integrating directly with Jetty HttpClient.
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3233:
URL: https://github.com/apache/solr/pull/3233#issuecomment-2693387135
The JWT test failure isn't related to this PR, I think; the same error [occurred previously](https://develocity.apache.org/s/bh6up7pafkzlq/tests/task/:solr:modules:jwt-auth:test/details/org.apache.solr.security.jwt.JWTAuthPluginIntegrationTest/mockOAuth2Server?top-execution=1).
The TestTaskManagement failure is weird; I'm looking closer at it but doesn't seem related (yet). No history of failures (that are relevant).
Meanwhile a bunch of test runs passed without failure so I'm feeling good about not having introduced a regression.
--
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
Re: [PR] SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3233: SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory
URL: https://github.com/apache/solr/pull/3233
--
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