kotman12 commented on PR #4974:
URL: https://github.com/apache/solr/pull/4974#issuecomment-6088797597
> Excellent work Luke!
>
> My only reservation is wether this belongs directly on HttpJettySolrClient
or could be a adjunct in some way for use only within Solr. But I saw the code
comment about wanting to ensure the async callbacks happen with the abort
callback in a way we control, so that basically rules out decoupling. That's
fine. It overall is rather light-weight, any way.
Thanks David!
Re making this a "private adjunct":
This is an excellent question! I'll start with that I don't agree it
_should_ be an "adjunct" for internal use only (btw I am assuming internal
"adjunct" means a custom listener factory you simply pass in, correct me if I
am mischaracterizing your idea). I think `abort`/`isAborted` is a natural API
to have. Most HTTP clients have some form of it so why make life difficult for
solrj users (such as ourselves 😃)? Perhaps you were also suggesting that
external users that need to abort could write their own custom abort listeners
for the same effect?
All that being said it is technically possible, even with the permit
tracking dilemma. You'd just have to make the `AsyncTracker` listener a special
case that is guaranteed to be invoked before (for both the `onQueued` and
`onComplete` cases) your custom private listeners. What makes it awkward is
that the existing listeners expect onQueued to be fired from the request-making
thread and not from the callback (really confusing). So you'd have to
disambiguate the new interface somehow.
But the most interesting thing is this raises the problem of listener
factory sharing:
```
public Builder withHttpClient(HttpJettySolrClient httpJettySolrClient) {
super.withHttpClient(httpJettySolrClient);
this.httpClient = httpJettySolrClient.httpClient;
if (this.idleTimeoutMillis == null) {
this.idleTimeoutMillis = httpJettySolrClient.idleTimeoutMillis;
}
if (this.listenerFactories == null) {
this.listenerFactories = httpJettySolrClient.listenerFactory;
}
if (this.executor == null) {
this.executor = httpJettySolrClient.executor;
}
return this;
}
```
If your request tracking "adjunct" (which I, again, assume is a listener
factory) gets added to a listener factory list then it will incidentally get
added to all http clients that are in its "copy-tree". Perhaps this should be a
copy-on write? Should we make this a separate ticket?
Btw, If we are really paranoid about backwards compatibility we could mark
it as `@lucene.experimental` though not sure how big of a concern that even is.
Worst case it moves to a no-op if somehow Jetty yanks the underlying hook
(though I highly doubt it).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
