SOLR-17688: Http2SolrClient: use Request.Listener not HttpListenerFactory

[dsmiley]

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

[dsmiley]

A test run passed. But a previous run without substantive changes failed in JWTAuthPluginIntegrationTest and TestTaskManagement; don't know why yet. The former is relevant to these changes (auth stuff). I'll want to do more testing to get more confidence.

[dsmiley]

this listener no per-request state so can simply add this listener to client level.

[dsmiley]

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.

[dsmiley]

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

[dsmiley]

because I removed client.getProtocolHandlers() now that HttpClient is exposed

[dsmiley]

The JWT test failure isn't related to this PR, I think; the same error occurred previously.
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.

[dsmiley]

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.

[stillalex]

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

[dsmiley]

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

[gerlowskija]

Awesome - this looks like a nice little cleanup!

[dsmiley]

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.