SOLR-18332: Remove reliance on deprecated 'qt' param in a first batch of tests - #4699
Conversation
… of tests Convert tests that set the 'qt' request parameter directly on a SolrQuery/SolrParams object to instead select the request handler via QueryRequest's path-based constructor or an explicit handler argument. Involves minor method-overload changes to SolrTestCaseJ4 and BaseDistributedSearchTestCase.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
thanks for continuing the crusade
|
|
||
| @Override | ||
| protected QueryResponse queryRandomShard(ModifiableSolrParams params) | ||
| protected QueryResponse queryRandomShard(String requestHandler, ModifiableSolrParams params) |
There was a problem hiding this comment.
maybe out of scope but it's a shame to see ModifiableSolrParams instead of simply SolrParams
| @@ -699,10 +698,11 @@ public void testErrorHandling() throws Exception { | |||
| SolrClient client = getSolrClient(); | |||
|
|
|||
| SolrQuery query = new SolrQuery(); | |||
There was a problem hiding this comment.
SolrQuery should only be used for SearchHandler.
| } | ||
| } | ||
|
|
||
| protected QueryResponse queryRandomShard(ModifiableSolrParams params) |
There was a problem hiding this comment.
Please just refer to SolrParams here
| * <p>WARNING: tests should generally not call this as it doesn't compare to the control client | ||
| */ | ||
| protected QueryResponse queryRandomShard(ModifiableSolrParams params) | ||
| protected QueryResponse queryRandomShard(String requestHandler, ModifiableSolrParams params) |
There was a problem hiding this comment.
No ModifiableSolrParams please
There was a problem hiding this comment.
The ModifiableSolrParams usage pre-exists this PR; it's not something this PR introduces. For scope reasons I'm a little leery of bogging down the qt effort in other unrelated refactors unless you feel especially strongly?
(See my PR-level comment asking a similar question.)
| try { | ||
| QueryResponse rsp1 = client.query(new ModifiableSolrParams(params)); | ||
| QueryResponse rsp1 = | ||
| new QueryRequest(requestHandler, new ModifiableSolrParams(params)) |
|
Hey @dsmiley - thanks for the review. I agree with all your comments by and large, but have scope concerns. Specifically: I'd love for all of our test API calls to use the correct 'SolrRequest' type (e.g. LukeRequest for So I wanted to ask: how strongly do you feel about fixing both things in the same sweep of the codebase? If you feel strongly I'll make slower progress but also address this sort of stuff. But if you don't, I'll stick pretty strictly to "just" ripping out qt as my initial pass here did. |
|
RE the proper request type -- not blocking. But for most of these spots, it's just changing the very same line you are already touching. I did similar work days ago -- I was modifying certain callers of QueryRequest to use the path. While I was at it, I also switched some to GSR as appropriate at the same call-site. |
|
I've addressed a number of the "right SolrRequest/SolrResponse" places and it did end up being easy in some (but not all) cases. The ModifiableSolrParams stuff I left untouched as that preexists this PR and would be a larger change. |
|
Test and check pass locally fwiw |
|
I love it! Now go to the beach ;-) |
3 participants
Convert tests that set the 'qt' request parameter directly on a
SolrQuery/SolrParams object to instead select the request handler via
QueryRequest's path-based constructor or an explicit handler argument.
Involves minor method-overload changes to SolrTestCaseJ4 and
BaseDistributedSearchTestCase.