SOLR-18072: Refactor CollectionApiCommands to add expandable context #4046
Conversation
github-actions
bot
added
test-framework
client:solrj
tool:build
tests
cat:cloud
cat:api
labels
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
I overall like the proposal of an Admin API context class for these APIs.
I've been looking in this PR for where SubResponseAccumulatingJerseyResponse is defined but can't find it; maybe because Safari/GitHub is completely choking on this massive PR. I don't love the name of that thing. Can you please self-review to identify it?
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
Show resolved
Hide resolved
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java
Outdated
Show resolved
Hide resolved
| CollectionAdminRequest.RequestStatus requestStatus = | ||
| CollectionAdminRequest.requestStatus(asyncId); | ||
| CollectionAdminRequest.RequestStatusResponse rsp = null; | ||
| TimeOut timeoutCheck = new TimeOut(timeout, TimeSource.NANO_TIME); |
There was a problem hiding this comment.
hmm; we also have org.apache.solr.common.util.RetryUtil utility. Maybe that should at least cross-link.
There was a problem hiding this comment.
I can switch over to that.
| import org.mockito.ArgumentCaptor; | ||
|
|
||
| /** | ||
| * Abstract test class to setup shared mocks for unit testing v2 API calls that go to the Overseer. |
There was a problem hiding this comment.
Overseer or DistributedCollectionConfigSetCommandRunner, fixed.
There was a problem hiding this comment.
Based on the content here, this seems actually for SolrCloud. If so; maybe the name or package should reflect that. Any way, this class is peculiar to me.
There was a problem hiding this comment.
I could move it, but all of these test classes already lived here, and this is just an abstract (will change) class to help with the mocking for most of the other test classes in this package.
This response class already exists, and the method that populated it already existed too. I just expanded the use of it, so that we weren't doing the same thing across each API independently. That's why you can't find it in the PR. We can change the name independently if you dislike it, but not really in the scope of this PR. |
| } | ||
| fail("Async request " + asyncId + " did not complete within duration: " + timeout.toString()); | ||
| return rsp; | ||
| final AtomicReference<CollectionAdminRequest.RequestStatusResponse> rsp = |
There was a problem hiding this comment.
This looks worse. I guess TimeOut is simpler in part due to no try-catch needed in some cases (like this) and no AtomicReference needed either. Dodging the try-catch need is unusual... normally a failing request propagates an exception but I see the API you are using doesn't do that. I think that's a known GAP in our APIs that I had been chatting with Jason about solving.
Any way, do as you please.
There was a problem hiding this comment.
Yeah, agreed.
Ok at first I was going to just revert the change. But then I decided adding an additional retry utility that would return a response, that is validated, would be better. The method looks MUCH better now.
2 participants
https://issues.apache.org/jira/browse/SOLR-18072
There are no functionality changes here. Just test improvements, and refactoring of the APIs.
I'm also happy to call
AdminCmdContextsomething else, that's a very easy change.