dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r596508662
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -211,6 +215,73 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd,
SolrResourceLoader loa
*/
public abstract String configSetName(CoreDescriptor cd);
+ /**
+ * Check whether a config exists in Zookeeper
Review comment:
Not _necessarily_ "in Zookeeper" :-)
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -211,6 +215,73 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd,
SolrResourceLoader loa
*/
public abstract String configSetName(CoreDescriptor cd);
+ /**
+ * Check whether a config exists in Zookeeper
+ *
+ * @param configName the config to check existance on
+ * @return whether the config exists or not
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract Boolean configExists(String configName) throws IOException;
+
+ /**
+ * Delete a config in ZooKeeper
+ *
+ * @param configName the config to delete
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract void deleteConfigDir(String configName) throws IOException;
Review comment:
I'm dubious that "Dir" needs to be in any of these method names
##########
File path:
solr/core/src/test/org/apache/solr/cloud/api/collections/SimpleCollectionCreateDeleteTest.java
##########
@@ -114,15 +114,15 @@ public void testDeleteAlsoDeletesAutocreatedConfigSet()
throws Exception {
// config for this collection is '.AUTOCREATED', and exists
globally
assertTrue(configName.endsWith(".AUTOCREATED"));
-
assertTrue(ZkConfigSetService.listConfigs(cloudClient.getZkStateReader().getZkClient()).contains(configName));
+
assertTrue(ZkMaintenanceUtils.listConfigs(cloudClient.getZkStateReader().getZkClient()).contains(configName));
Review comment:
Let's not have `listConfigs` in ZkMaintenanceUtils.
Earlier in this test, I see a method call you could use instead at this line
--
`cloudClient.getZkStateReader().getZkClient().exists(ZkStateReader.COLLECTIONS_ZKNODE
+ "/" + collectionName, false)`
##########
File path:
solr/solrj/src/test/org/apache/solr/client/ref_guide_examples/ZkConfigFilesTest.java
##########
@@ -38,12 +37,13 @@
*/
public class ZkConfigFilesTest extends SolrCloudTestCase {
- private static final int ZK_TIMEOUT_MILLIS = 10000;
+ private static ConfigSetService configSetService;
Review comment:
A static field (that isn't a constant) like this needs to be null'ed in
AfterClass for a test. It may hold onto resources longer than we want; maybe
even leading to problems.
Alternatively, let's add a `getConfigSetService()` method to the
MiniSolrCloudService that makes this convenient? Only if it might be used in a
number of places.
##########
File path: solr/core/src/java/org/apache/solr/util/SolrCLI.java
##########
@@ -102,14 +102,7 @@
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.cloud.ZkConfigSetService;
import org.apache.solr.common.SolrException;
-import org.apache.solr.common.cloud.ClusterState;
-import org.apache.solr.common.cloud.DocCollection;
-import org.apache.solr.common.cloud.UrlScheme;
-import org.apache.solr.common.cloud.Replica;
-import org.apache.solr.common.cloud.Slice;
-import org.apache.solr.common.cloud.SolrZkClient;
-import org.apache.solr.common.cloud.ZkCoreNodeProps;
-import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.cloud.*;
Review comment:
Configure IntelliJ to not do that. Use "Google Java Format" plugin to
help. This is a relatively recent direction of the project.
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -233,7 +233,7 @@ protected int loadElevationConfiguration(SolrCore core)
throws Exception {
ZkController zkController = core.getCoreContainer().getZkController();
if (zkController != null) {
// TODO : shouldn't have to keep reading the config name when it has
been read before
- configFileExists =
zkController.configFileExists(zkController.getZkStateReader().readConfigName(core.getCoreDescriptor().getCloudDescriptor().getCollectionName()),
configFileName);
+ configFileExists =
zkController.getCoreContainer().getConfigSetService().configExists(configFileName);
Review comment:
Nope :-)
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -211,6 +215,73 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd,
SolrResourceLoader loa
*/
public abstract String configSetName(CoreDescriptor cd);
+ /**
+ * Check whether a config exists in Zookeeper
+ *
+ * @param configName the config to check existance on
+ * @return whether the config exists or not
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract Boolean configExists(String configName) throws IOException;
+
+ /**
+ * Delete a config in ZooKeeper
+ *
+ * @param configName the config to delete
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract void deleteConfigDir(String configName) throws IOException;
+
+ /**
+ * Copy a config in ZooKeeper
+ *
+ * @param fromConfig the config to copy from
+ * @param toConfig the config to copy to
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract void copyConfigDir(String fromConfig, String toConfig)
throws IOException;
+
+ /**
+ * Copy a config in ZooKeeper
+ *
+ * @param fromConfig the config to copy from
+ * @param toConfig the config to copy to
+ * @param copiedToZkPaths should be an empty Set, will be filled in by
function
+ * with the paths that were actually copied to.
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract void copyConfigDir(String fromConfig, String toConfig,
Set<String> copiedToZkPaths) throws IOException;
+
+ /**
+ * Upload files from a given path to a config in Zookeeper
+ *
+ * @param dir {@link java.nio.file.Path} to the files
+ * @param configName the name to give the config
+ * @throws IOException if an I/O error occurs or the path does not exist
+ */
+ public abstract void uploadConfigDir(Path dir, String configName) throws
IOException;
+
+ /**
+ * Upload matching files from a given path to a config in Zookeeper
+ *
+ * @param dir {@link java.nio.file.Path} to the files
+ * @param configName the name to give the config
+ * @param filenameExclusions files matching this pattern will not be uploaded
+ * @throws IOException if an I/O error occurs or the path does not exist
+ */
+ public abstract void uploadConfigDir(Path dir, String configName, Pattern
filenameExclusions) throws IOException;
Review comment:
Is this version really necessary? Shouldn't we _always_ filter
dot-files?
##########
File path:
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIZkFailure.java
##########
@@ -111,13 +111,13 @@ public void testCreateZkFailure() throws Exception {
AbstractZkTestCase.TIMEOUT, AbstractZkTestCase.TIMEOUT, null);
try {
- assertFalse(ZkConfigSetService.configExists(zkClient, CONFIGSET_NAME));
+
assertFalse(solrCluster.getOpenOverseer().getCoreContainer().getConfigSetService().configExists(CONFIGSET_NAME));
Review comment:
is the `getOpenOverseer()` part necessary? Seems dubious.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -211,6 +215,73 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd,
SolrResourceLoader loa
*/
public abstract String configSetName(CoreDescriptor cd);
+ /**
+ * Check whether a config exists in Zookeeper
+ *
+ * @param configName the config to check existance on
+ * @return whether the config exists or not
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract Boolean configExists(String configName) throws IOException;
Review comment:
Also to be more consistent with the other method names, don't start the
method with "config"; put something before. I propose "checkConfigExists"
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java
##########
@@ -830,15 +826,15 @@ public void clean(String path, Predicate<String>
nodeFilter) throws InterruptedE
}
public void upConfig(Path confPath, String confName) throws IOException {
- ZkMaintenanceUtils.uploadToZK(this, confPath, CONFIGS_ZKNODE + "/" +
confName, UPLOAD_FILENAME_EXCLUDE_PATTERN);
+ ZkMaintenanceUtils.uploadToZK(this, confPath,
ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName,
ZkMaintenanceUtils.UPLOAD_FILENAME_EXCLUDE_PATTERN);
Review comment:
I think `ZkMaintenanceUtils.UPLOAD_FILENAME_EXCLUDE_PATTERN` -- the
constant there should have a name with "CONFIGS" in there, perhaps at the
front. It used to be on ZkConfigManager which implies a context of configs but
this isn't so for ZkMaintenanceUtils. On the other hand, wow that'd be an
even longer name. At least add a comment on it.
##########
File path:
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java
##########
@@ -56,6 +59,46 @@ public String configSetName(CoreDescriptor cd) {
return (cd.getConfigSet() == null ? "instancedir " : "configset ") +
locateInstanceDir(cd);
}
+ @Override
+ public Boolean configExists(String configName) throws IOException {
+ return null;
Review comment:
No! If you don't implement something, throw
UnsupportedOperationException. I configure my IDE such that auto-generated
methods throw this.
But some of these on file system, I think you should support. configExists
in particular, is important IMO. listConfigs as well. I think we could then
actually view configs from the existing HTTP config sets API. You should try!
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -211,6 +215,73 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd,
SolrResourceLoader loa
*/
public abstract String configSetName(CoreDescriptor cd);
+ /**
+ * Check whether a config exists in Zookeeper
+ *
+ * @param configName the config to check existance on
+ * @return whether the config exists or not
+ * @throws IOException if an I/O error occurs
+ */
+ public abstract Boolean configExists(String configName) throws IOException;
Review comment:
Why `Boolean` and not `boolean`? Should it change or if not then
document.
----------------------------------------------------------------
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:
[email protected]
