dsmiley commented on code in PR #2363: URL: https://github.com/apache/solr/pull/2363#discussion_r1534052101
########## solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java: ########## @@ -345,6 +348,41 @@ protected void init() throws Exception { action = PASSTHROUGH; } + /** + * Lookup the collection from the collection string (maybe comma delimited). Also sets {@link + * #collectionsList} by side-effect. if {@code secondTry} is false then we'll potentially + * recursively try this all one more time while ensuring the alias and collection info is sync'ed + * from ZK. + */ + protected DocCollection resolveDocCollection(List<String> collectionsList) { + if (!cores.isZooKeeperAware()) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "Solr not running in cloud mode "); + } + ZkStateReader zkStateReader = cores.getZkController().getZkStateReader(); + Supplier<DocCollection> logic = + () -> { + String collectionName = collectionsList.get(0); // first + // TODO an option to choose another collection in the list if can't find a local replica + // of the first? + return zkStateReader.getClusterState().getCollectionOrNull(collectionName); + }; + + DocCollection docCollection = logic.get(); + if (docCollection != null) { + return docCollection; + } + // ensure our view is up to date before trying again + try { + zkStateReader.aliasesManager.update(); + zkStateReader.forceUpdateCollection(collectionsList.get(0)); Review Comment: again; the choice to do this for the first collection seems repeated here (see earlier `collectionsList.get(0)`). Would be clearer to do this once and have a var like "collectionName". ########## solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java: ########## @@ -345,6 +348,41 @@ protected void init() throws Exception { action = PASSTHROUGH; } + /** + * Lookup the collection from the collection string (maybe comma delimited). Also sets {@link + * #collectionsList} by side-effect. if {@code secondTry} is false then we'll potentially + * recursively try this all one more time while ensuring the alias and collection info is sync'ed + * from ZK. + */ + protected DocCollection resolveDocCollection(List<String> collectionsList) { + if (!cores.isZooKeeperAware()) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "Solr not running in cloud mode "); + } + ZkStateReader zkStateReader = cores.getZkController().getZkStateReader(); + Supplier<DocCollection> logic = + () -> { + String collectionName = collectionsList.get(0); // first + // TODO an option to choose another collection in the list if can't find a local replica + // of the first? Review Comment: I know you are just moving code but these 3 lines seem like they don't belong inside the supplier; should be done before hand. Won't need to be repeated. ########## solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java: ########## @@ -144,11 +144,22 @@ public static ClusterState createFromJsonSupportingLegacyConfigName( @Override public ClusterState.CollectionRef getState(String collection) { ClusterState clusterState = getZkStateReader().getClusterState(); - if (clusterState != null) { - return clusterState.getCollectionRef(collection); - } else { + if (clusterState == null) { Review Comment: When would this happen? If it's a startup/initialization scenario, maybe getClusterState() should wait until it exists? Okay to defer this question to another PR as this concern pre-existed albeit maybe it could be another reason why we aren't resolving the collection. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org