[
https://issues.apache.org/jira/browse/SOLR-6625?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14244185#comment-14244185
]
Per Steffensen commented on SOLR-6625:
--------------------------------------
Had a few hours for a look at the latest patch. Not a lot of time so I might
very well have overlooked something, so please bear with me. But from what I
got out of it I have the following comments
It seems you can control the used callbacks in two ways
* 1) Through {{solr.xml}}.
** Request that {{SolrDispatchFilter}} forwards (using {{remoteQuery}})
** Update requests sent through {{UpdateShardHandler}} -
{{DistributedUpdateProcessor}}, {{SnapPuller}}, {{PeerSync}}, {{SyncStrategy}}
...
** {{Overseer}} requests (through {{UpdateShardHandler}} via {{ZkController}})
* 2) Through VM parameter. Used for every request sent - from Solr nodes and
clients or whatever is running in the JVM sending request using
{{HttpSolrServer}} (or {{ConcurrentUpdateSolrServer}}). Believe, for the
requests mentioned under 1) above, both callbacks (the one from {{solr.xml}}
AND the one from VM parameter) will be triggered - is that intentional? (it is
fine for me)
I think, that if you want to support setting the callback through {{solr.xml}}
it should be used for all requests sent out of a Solr node - including e.g.
search requests to shards issued from {{SearchHandler}} (by code inspection it
seems to me that it will not currently be included here). Regarding this
principle of using the {{solr.xml}}-callback for ALL requests going out of a
Solr node, I would really like to see some testing that makes sure this
principle is fulfilled now and that it is not broken in the future (the day
after SOLR-6625 is committed and forgotten about, someone adds a new place in
the code sending requests going out of Solr nodes and forgets to call the
callbacks). I wanted to do the same in SOLR-4470 (making sure that no requests
going out of Solr nodes do not have credentials added - not it the code as it
is today nor in the code as it changes in the future). I achieved it by adding
a protecting layer around every Solr node ever started during testing, and
assuming that the test-suite triggers every type of requests going out of Solr
nodes, that will basically ensure that no one forgets to propagate credentials
when extending the code in the future (those requests will fail and the test
will likely fail). I would really like to see something similar here, verifying
that {{solr.xml}}-callback is triggered for every request going out of a Solr
node.
Maybe you should consider the naming of {{distribCount}} and {{forwardCount}}
in {{BasicDistributedZk2Test.HttpClientRequestCallbackCounter}}. They are very
confusing to me. {{distribCount}} seem to count update-requests (they are kinda
forwarded) and {{forwardCount}} seem to count everything else including request
from e.g. {{Overseer}} (they are among those that can not be characterized as
forwarded)
I know you are not keen on cleaning up while adding a new feature (I was told
that we have to do cleanup in different JIRAs). Personally I am very much
against that, because no one will ever make a JIRA just to clean up (at least,
looking at the code-base I see that it does not happen enough). Assuming we
want to do a little cleanup (in that code that is touched anyway in this JIRA)
I have the following suggestions
* Change {{ConcurrentUpdateSolrServer}} to use {{HttpSolrServer}} or at least
make them share common code. Then we do not have "remember" to add
callback-support in both when we solve this SOLR-6625
* In general we should do some "Separation of Concerns" here and create a
single component X (e.g. {{SolrNodeOutgoingRequestSender}}) concerned with
sending requests out of Solr nodes. Make X use callback from {{solr.xml}}.
** Let {{SolrDispatchFilter}}, {{UpdateShardHandler}} and whatever sends
requests out of Solr nodes use that component X, or something based on or using
it
** It is strange that {{Overseer}} uses {{UpdateShardHandler}} - {{Overseer}}
is not doing updates. Either rename {{UpdateShardHandler}} to something that
does not signal that it is only used for update-requests, or let {{Overseer}}
use something else based on component X
Instead of adding callback-calls numerous places in the code not knowing if
some place was forgotten, polluting the code even more and leaving the same
kind of problems for others doing similar things in the future, IMHO SOLR-6625
should be solved by refactoring the code so that the actual feature of
SOLR-6625 is ridiculously easy to add
* Separating the general concern of sending request from solr-code (server or
client) to one single component Y. ({{HttpSolrServer}} or a class containing
the code shared between it and {{ConcurrentUpdateSolrServer}} is fine for now).
It is easy to make a test that ensures that {{HttpClient.execute}} only occurs
one single place in the (non-test) code
* Separating the concern of sending requests going out from Solr nodes to one
single component X (of course using Y). Make the test-suite verify that
Solr-nodes never ever send request bypassing this component Y - maybe inspired
by the way it is done in SOLR-4470 (here the test is verifying that Solr-nodes
never ever sends requests without adding credentials - potato potato)
* Now it is ridiculously easy to solve SOLR-6625 (and you have used your
strength cleaning up the code instead of polluting it even more): Make X
capable of taking a list L of callbacks, and let it call those callbacks plus
the VM-parameter based callbacks. Make sure Y passes the {{solr.xml}}-callbacks
in this list L when it uses X. Testing is almost superfluous, because you know
that requests from solr-code are always sent using X (which calls VM-parameter
callbacks) and that requests going out of Solr nodes are already sent using Y
(which makes sure X calls {{solr.xml}}-callbacks). Maybe we few simple tests,
e.g. a test that X always uses VM-parameter callbacks, and a test that Y
actually makes X use the {{solr.xml}}-callbacks.
Yes, I deliberately said callbackS. I think there should be support for a list
of callbacks, so that people using this general feature will be able to do
proper separation of concerns if they want to do several conceptually different
things using callbacks
If SOLR-6625 should be able to be used directly for a SOLR-4470 solution, we
need to include the "super-request" (the request that indirectly triggered the
request about to be fired) in the call to the callback. I understand if you do
not want to deal with that now - we can deal with that later.
I really like the nice general feature you are making here - one vote from me!
Looking very much forward to see how it ends.
> HttpClient callback in HttpSolrServer
> -------------------------------------
>
> Key: SOLR-6625
> URL: https://issues.apache.org/jira/browse/SOLR-6625
> Project: Solr
> Issue Type: Improvement
> Components: SolrJ
> Reporter: Gregory Chanan
> Assignee: Gregory Chanan
> Priority: Minor
> Attachments: SOLR-6625.patch, SOLR-6625.patch, SOLR-6625.patch,
> SOLR-6625.patch, SOLR-6625.patch, SOLR-6625.patch
>
>
> Some of our setups use Solr in a SPNego/kerberos setup (we've done this by
> adding our own filters to the web.xml). We have an issue in that SPNego
> requires a negotiation step, but some HttpSolrServer requests are not
> repeatable, notably the PUT/POST requests. So, what happens is,
> HttpSolrServer sends the requests, the server responds with a negotiation
> request, and the request fails because the request is not repeatable. We've
> modified our code to send a repeatable request beforehand in these cases.
> It would be nicer if HttpSolrServer provided a pre/post callback when it was
> making an httpclient request. This would allow administrators to make
> changes to the request for authentication purposes, and would allow users to
> make per-request changes to the httpclient calls (i.e. modify httpclient
> requestconfig to modify the timeout on a per-request basis).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
