[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager

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

dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r596028789



##########
File path: solr/contrib/prometheus-exporter/src/test/org/apache/solr/prometheus/scraper/SolrFileSystemConfigSetServiceScraperTest.java
##########
@@ -40,7 +40,7 @@
 import org.junit.BeforeClass;
 import org.junit.Test;
 
-public class SolrStandaloneScraperTest extends RestTestBase {
+public class SolrFileSystemConfigSetServiceScraperTest extends RestTestBase {

Review comment:
       don't change

##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerConfigSetMessageHandler.java
##########
@@ -297,13 +296,12 @@ private void createConfigSet(ZkNodeProps message) throws IOException {
 
     String baseConfigSetName = message.getStr(BASE_CONFIGSET, DEFAULT_CONFIGSET_NAME);
 
-    ZkConfigManager configManager = new ZkConfigManager(zkStateReader.getZkClient());
-    if (configManager.configExists(configSetName)) {
+    if (ZkConfigSetService.configExists(zkStateReader.getZkClient(), configSetName)) {

Review comment:
       Everywhere code specifically calls `ZkConfigSetService` is breaking the abstraction (plug-ability) we are trying to create.  Thus you need to get ahold of a ConfigSetService impl from CoreContainer and call general methods on that which might be implemented in any ways an implementation desired (not necessarily ZK).

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -897,7 +897,7 @@ private static void bootstrapDefaultConfigSet(SolrZkClient zkClient) throws Keep
             , "intended to be the default. Current 'solr.default.confdir' value:"
             , System.getProperty(SolrDispatchFilter.SOLR_DEFAULT_CONFDIR_ATTRIBUTE));
       } else {
-        ZkMaintenanceUtils.upConfig(zkClient, Paths.get(configDirPath), ConfigSetsHandler.DEFAULT_CONFIGSET_NAME);
+        ZkConfigSetService.uploadConfigDir(zkClient, Paths.get(configDirPath), ConfigSetsHandler.DEFAULT_CONFIGSET_NAME);

Review comment:
       Definitely should use ConfigSetService generically (not static methods).  I also wonder what this `getConfigFileData` is even doing here -- seems like something that maybe belongs in ConfigSetService?

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -795,7 +795,7 @@ public SolrCloudManager getSolrCloudManager() {
    */
   public byte[] getConfigFileData(String zkConfigName, String fileName)

Review comment:
       not used; remove

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -0,0 +1,324 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.ref.WeakReference;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.regex.Pattern;
+
+import org.apache.solr.client.solrj.cloud.SolrCloudManager;
+import org.apache.solr.cloud.api.collections.CreateCollectionCmd;
+import org.apache.solr.common.ConfigNode;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.cloud.ZooKeeperException;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.ConfigSetProperties;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreDescriptor;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * SolrCloud Zookeeper ConfigSetService impl.
+ */
+public class ZkConfigSetService extends ConfigSetService {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+  private Map<String, ConfigCacheEntry> cache = new ConcurrentHashMap<>();
+  private final ZkController zkController;
+  /** ZkNode where named configs are stored */
+  public static final String CONFIGS_ZKNODE = "/configs";
+  public static final String UPLOAD_FILENAME_EXCLUDE_REGEX = "^\\..*$";
+  public static final Pattern UPLOAD_FILENAME_EXCLUDE_PATTERN = Pattern.compile(UPLOAD_FILENAME_EXCLUDE_REGEX);
+
+  public ZkConfigSetService(SolrResourceLoader loader, boolean shareSchema, ZkController zkController) {
+    super(loader, shareSchema);
+    this.zkController = zkController;
+  }
+
+  public void storeConfig(String resource, ConfigNode config, int znodeVersion) {

Review comment:
       BTW you can remove storeConfig, getConfig, and the cache, and ConfigCacheEntry.  This is dead code added recently (a bit sloppy, I know).

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -2047,7 +2044,7 @@ public static void bootstrapConf(SolrZkClient zkClient, CoreContainer cc) throws
         confName = coreName;
       Path udir = cd.getInstanceDir().resolve("conf");
       log.info("Uploading directory {} with name {} for solrCore {}", udir, confName, coreName);
-      configManager.uploadConfigDir(udir, confName);
+      ZkConfigSetService.uploadConfigDir(zkClient, udir, confName);

Review comment:
       should use configSetService

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -231,9 +230,8 @@ public static void main(String[] args) throws InterruptedException,
             stdout.println("A chroot was specified in zkHost but the znode doesn't exist. ");
             System.exit(1);
           }
-          ZkConfigManager configManager = new ZkConfigManager(zkClient);
           final Pattern excludePattern = Pattern.compile(excludeExpr);
-          configManager.uploadConfigDir(Paths.get(confDir), confName, excludePattern);
+          ZkConfigSetService.uploadConfigDir(zkClient, Paths.get(confDir), confName, excludePattern);

Review comment:
       I think ZkConfigSetService should not offer a configurable exclude pattern.  It should always bee the one that excludes dot-files.  Do you agree?

##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -755,7 +755,7 @@ public void giveupLeadership(CoreDescriptor cd) {
    */
   public boolean configFileExists(String collection, String fileName)
       throws KeeperException, InterruptedException {
-    Stat stat = zkClient.exists(ZkConfigManager.CONFIGS_ZKNODE + "/" + collection + "/" + fileName, null, true);
+    Stat stat = zkClient.exists(ZkConfigSetService.CONFIGS_ZKNODE + "/" + collection + "/" + fileName, null, true);

Review comment:
       This method (configFileExists) shouldn't even be here.  It's only called by QueryElevationComponent.  That component should instead use the resourceLoader to try to resolve the file.




----------------------------------------------------------------
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.

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