SOLR-18360: remove HttpJettySolrClient.addListenerFactory via a repointable wrapper - #4780
Conversation
…istenerFactory) The ticket asked to remove this instance method in favor of the Builder-only equivalent, but PKIAuthenticationPlugin (implementing HttpClientBuilderPlugin) attaches a listener to already-built clients across HttpShardHandlerFactory/UpdateShardHandler/HttpSolrClientProvider from CoreContainer.setupHttpClientForAuthPlugin -- a path that can re-fire on security.json hot-reload, on clients that other long-lived objects (e.g. HttpShardHandlerFactory's own loadbalancer) already reference. Rebuilding via the Builder instead would desync those references. Migrated the one call site that WAS just construction-time convenience (HttpShardHandlerFactory's own defaultClient) to the Builder method; left the instance method for the case it actually serves.
|
@epugh flagging this un-deprecation (not a removal) -- see PR description for why the instance method is still needed. |
…e-addlistenerfactory
|
Maybe a mutable ListenerFactory wrapper could be repointed (a hack), instead of exposing a mutable method on an important SolrJ class. |
Per dsmiley's review: MutableListenerFactory repoints via a single volatile write instead of mutating a shared List -- fixes a real thread-safety issue and a listener-accumulation bug on security.json reload, and lets us actually remove the method per the original ticket instead of un-deprecating.
serhiy-bzhezytskyy
changed the title
|
Implemented -- added |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
I like this!
(please credit me in the changelog)
|
|
||
| public default void setup(HttpJettySolrClient client) {} | ||
| /** May be called more than once; each call replaces the previous listener, not adds to it. */ | ||
| public default void setup(MutableListenerFactory listenerFactory) {} |
There was a problem hiding this comment.
If someone is writing a HttpClientBuilderPlugin, maybe this setup method should expose the HttpJettySolrClient? On the other hand, if the client instance is really many/variable but they all share a MutableListenerFactory then it'd be the wrong design. Yeah ... just speaking out loud.
CC @gerlowskija @janhoy the change proposed here changes our API for customizing HttpClientBuilder. It's labeled "lucene.experiment" so I suppose we can change at-will but I want to raise attention on this. It will belong in the changelog.
|
@epugh does this need a changelog entry, or can you add |
2 participants
Removes
HttpJettySolrClient.addListenerFactory(HttpListenerFactory)per the original ticket, instead of un-deprecating it as this PR first did.PKIAuthenticationPluginneeds to attach a listener to a client that's already built and already referenced elsewhere (HttpShardHandlerFactory'sdefaultClientis captured byloadbalancerat construction time) -- so a straight removal broke security wiring. @dsmiley's suggestion: don't expose a mutable method on the client itself, use a small repointable wrapper instead.MutableListenerFactoryis aHttpListenerFactorywhose delegate is repointed via a single volatile write.HttpJettySolrClientis registered with one at construction time (immutable, viaBuilder.addListenerFactory, same as before) and stays fully immutable itself --listenerFactoryfield isfinal, no instance mutator.HttpClientBuilderPlugin.setup()now takes theMutableListenerFactoryinstead of the client (it's@lucene.experimental, no compat concern there).This also fixes two real bugs the old instance method had:
Listwhile other threads iterate it on every inter-node request.security.jsonhot-reload -- each call only ever appended, never replaced.HttpShardHandlerFactory/UpdateShardHandler/HttpSolrClientProviderupdated to hold aMutableListenerFactoryfield and pass it tosetup()instead of the client. No changelog file yet -- flagging since this went from "un-deprecation" to an actual removal + new class; your call on whether one's warranted.50 tests, 0 failures.
TestPKIAuthenticationPlugin,BasicAuthIntegrationTest,MultiAuthPluginTestre-run and pass.AI-assisted (Claude Sonnet 5)