[GitHub] [solr] dsmiley commented on pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module

[GitBox <gi...@apache.org>]

dsmiley commented on PR #943:
URL: https://github.com/apache/solr/pull/943#issuecomment-1206936702

   PerReplicaStates is referenced in ClusterState, DocCollection, and Replica.  Based on my very limited understanding of PRS, it's a part of the state of a collection no matter how it's retrieved (via ZK or via SolrClient based ClusterStateProvider).  Am I right @noblepaul @chatman  ?
   
   I do like your suggestion of those 3 "Props" classes for constants.  How do we get there from here?  Directly, even if it changes import statement in lots more classes (I would assume)?  This is the easiest path.  Or do we leave that for another JIRA issue (soon) and for now just remove the nocommits about this to make precommit happy?  Also relatively easy but leaves some WIP.  Or go part way here & now -- add the new classes & constants but only use them in solrj-zookeeper right now and then migrate the rest in another issue?  More work.
   
   I would like to point out that this new separation did move some tests to solrj-zookeeper but it's only four -- that's it.  Many solrj tests indirectly use ZK and I suppose that's okay.  There is a compile time dependency on ZK from solrj tests that is unfortunate but couldn't be avoided in a few cases (without investing additional work):
   * StreamingTest.streamLocalTests  calls `zkStateReader.forceUpdateCollection`
   * TestCloudSolrClientConnections calls cluster.getZkClient()
   * org.apache.solr.client.solrj.impl.HttpClusterStateSSLTest#testHttpClusterStateWithSSL calls cluster.getZkClient() 
   Perhaps these might even move but on the other hand, it's not some big deal to add an explicit dependency on solrj tests to ZK when ZK is definitely already there at runtime via the solrj-zookeeper dependency.
   
   There is a weird dependency check problem that I can't figure out:
   ```
   Execution failed for task ':solr:solrj-zookeeper:analyzeTestClassesDependencies'.
   > Dependency analysis found issues.
     usedUndeclaredArtifacts
      - org.apache.solr:solrj-zookeeper:10.0.0-SNAPSHOT@
   ```
   So solrj-zookeeper's tests depend on solrj-zookeeper.  Well of course it does ;-)
   


-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org