dsmiley commented on code in PR #5314: URL: https://github.com/apache/solr/pull/5314#discussion_r4525862034
########## solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateReaderTest.java: ########## Review Comment: You didn't change ZkStateReader yet you add tests here. Are you enhancing ZkStateReader's tests or if not maybe these go elsewhere. ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java: ########## Review Comment: In your opinion, would it be clearer for alias resolution to move to the ClusterStateProvider abstraction? That might be bigger / out of scope, but it's on my mind. ########## solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java: ########## Review Comment: TBH I hate mocks; I don't trust them and they entangle future work on the code under test. Instead, I prefer to test the observed effect, using observability signals (logs, tracing, metrics) to detect the behavior we want to test for, when possible. See testHttpCspPerf which is a very important test, whose absence had meant working yet slow CloudSolrClient behavior. Could you consider adding a variation of this for the alias resolution matter? That was the perf regression bug that I hoped to see get added as a test (which had never happened). Wish such a test in place, we can feel much more confident about what we're doing in this PR. Hmm... it's easier to use the technique I describe for an HTTP based requests but less so for ZK interactions. As long as there's a log/metric we can see, we can do 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]
