You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/03/17 12:45:45 UTC
[GitHub] [solr] NazerkeBS opened a new pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
NazerkeBS opened a new pull request #23:
URL: https://github.com/apache/solr/pull/23
- `Standalone` class renamed to `FileSystemConfigSet`
- `CloudConfigSetService` class renamed to `ZkConfigSetService`
- Removed `ZkConfigManager`
- `TestZkConfigManager`renamed to `TestZkConfigSetService`
- All methods of `ZkConfigManager` are now `static` methods of `ZkConfigSetService`
- `ZkMaintenanceUtils` in solrj has also `uploadConfig`, `downloadConfig` and `listConfigs` methods which are the same in `ZkConfigSetService` from core
- `ZkClientClusterStateProvider` doesn't provide any upload or download configs functionalities anymore
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
----------------------------------------------------------------
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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603761380
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -296,51 +293,45 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
- private void downloadFromZK(SolrZkClient zkClient, String zkPath, URI dir) throws IOException {
- try {
- List<String> files = zkClient.getChildren(zkPath, null, true);
- for (String file : files) {
- List<String> children = zkClient.getChildren(zkPath + "/" + file, null, true);
- if (children.size() == 0) {
- log.debug("Writing file {}", file);
- byte[] data = zkClient.getData(zkPath + "/" + file, null, null, true);
- try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
- os.write(data);
- }
- } else {
- URI uri = repository.resolve(dir, file);
- if (!repository.exists(uri)) {
- repository.createDirectory(uri);
- }
- downloadFromZK(zkClient, zkPath + "/" + file, repository.resolve(dir, file));
+ private void downloadConfigToRepo(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> filePaths = configSetService.getAllConfigFiles(configName);
+ for (String filePath : filePaths) {
+ URI uri = repository.resolve(dir, filePath);
+ if (!filePath.endsWith("/")) {
+ log.debug("Writing file {}", filePath);
+ byte[] data = configSetService.downloadFileFromConfig(configName, filePath);
+ try (OutputStream os = repository.createOutput(uri)) {
+ os.write(data);
+ }
+ } else {
+ if (!repository.exists(uri)) {
+ repository.createDirectory(uri);
}
}
- } catch (KeeperException | InterruptedException e) {
- throw new IOException("Error downloading files from zookeeper path " + zkPath + " to " + dir.toString(),
- SolrZkClient.checkInterrupted(e));
}
}
- private void uploadToZk(SolrZkClient zkClient, URI sourceDir, String destZkPath) throws IOException {
+ private void uploadConfigToConfigRepo(ConfigSetService configSetService, URI sourceDir, String configName, String filePrefix) throws IOException {
Review comment:
I suggest naming `uploadConfigFromRepo` or `uploadConfigToSolrCloud`
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599578600
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -0,0 +1,319 @@
+/*
+ * 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.File;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+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.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.CoreContainer;
+import org.apache.solr.core.CoreDescriptor;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.servlet.SolrDispatchFilter;
+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 final ZkController zkController;
+ private final SolrZkClient zkClient;
+ /** 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(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.zkController = cc.getZkController();
+ this.zkClient = cc.getZkController().getZkClient();
+ try {
+ bootstrapDefaultConfigSet();
+ } catch (IOException e) {
+ log.error("Error in bootstrapping default config");
+ }
+ }
+
+ /** This is for ZkCLI and some tests */
+ public ZkConfigSetService(SolrZkClient zkClient) {
+ super(null, false);
+ this.zkController = null;
+ this.zkClient = zkClient;
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ final String colName = cd.getCollectionName();
+
+ // For back compat with cores that can create collections without the collections API
+ try {
+ if (!zkClient.exists(ZkStateReader.COLLECTIONS_ZKNODE + "/" + colName, true)) {
+ // TODO remove this functionality or maybe move to a CLI mechanism
+ log.warn("Auto-creating collection (in ZK) from core descriptor (on disk). This feature may go away!");
+ CreateCollectionCmd.createCollectionZkNode(zkController.getSolrCloudManager().getDistribStateManager(), colName, cd.getCloudDescriptor().getParams());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Interrupted auto-creating collection", e);
+ } catch (KeeperException e) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Failure auto-creating collection", e);
+ }
+
+ // The configSet is read from ZK and populated. Ignore CD's pre-existing configSet; only populated in standalone
+ final String configSetName;
+ try {
+ configSetName = zkController.getZkStateReader().readConfigName(colName);
+ cd.setConfigSet(configSetName);
+ } catch (KeeperException ex) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Trouble resolving configSet for collection " + colName + ": " + ex.getMessage());
+ }
+
+ return new ZkSolrResourceLoader(cd.getInstanceDir(), configSetName, parentLoader.getClassLoader(), zkController);
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes"})
+ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loader) {
+ try {
+ return ConfigSetProperties.readFromResourceLoader(loader, ".");
+ } catch (Exception ex) {
+ log.debug("No configSet flags", ex);
+ return null;
+ }
+ }
+
+ @Override
+ protected Long getCurrentSchemaModificationVersion(String configSet, SolrConfig solrConfig, String schemaFile) {
+ String zkPath = CONFIGS_ZKNODE + "/" + configSet + "/" + schemaFile;
+ Stat stat;
+ try {
+ stat = zkClient.exists(zkPath, null, true);
+ } catch (KeeperException e) {
+ log.warn("Unexpected exception when getting modification time of {}", zkPath, e);
+ return null; // debatable; we'll see an error soon if there's a real problem
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ if (stat == null) { // not found
+ return null;
+ }
+ return (long) stat.getVersion();
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return "configset " + cd.getConfigSet();
+ }
+
+ @Override
+ public boolean checkConfigExists(String configName) throws IOException {
+ try {
+ return zkClient.exists(CONFIGS_ZKNODE + "/" + configName, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void deleteConfigDir(String configName) throws IOException {
+ try {
+ zkClient.clean(CONFIGS_ZKNODE + "/" + configName);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig) throws IOException {
+ copyConfigDir(fromConfig, toConfig, null);
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException {
+ String fromConfigPath = CONFIGS_ZKNODE + "/" + fromConfig;
+ String toConfigPath = CONFIGS_ZKNODE + "/" + toConfig;
+ try {
+ copyData(copiedToZkPaths, fromConfigPath, toConfigPath);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error config " + fromConfig + " to " + toConfig,
+ SolrZkClient.checkInterrupted(e));
+ }
+ copyConfigDirFromZk(fromConfigPath, toConfigPath, copiedToZkPaths);
+ }
+
+ @Override
+ public void uploadConfigDir(Path dir, String configName) throws IOException {
+ zkClient.uploadToZK(dir, CONFIGS_ZKNODE + "/" + configName, ZkConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN);
+ }
+
+ @Override
+ public void downloadConfigDir(String configName, Path dir) throws IOException {
+ zkClient.downloadFromZK(CONFIGS_ZKNODE + "/" + configName, dir);
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ try {
+ return zkClient.getChildren(CONFIGS_ZKNODE, null, true);
+ } catch (KeeperException.NoNodeException e) {
+ return Collections.emptyList();
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error listing configs", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ private void bootstrapDefaultConfigSet() throws IOException {
+ if (this.checkConfigExists("_default") == false) {
+ String configDirPath = getDefaultConfigDirPath();
+ if (configDirPath == null) {
+ log.warn(
+ "The _default configset could not be uploaded. Please provide 'solr.default.confdir' parameter that points to a configset {} {}",
+ "intended to be the default. Current 'solr.default.confdir' value:",
+ System.getProperty(SolrDispatchFilter.SOLR_DEFAULT_CONFDIR_ATTRIBUTE));
+ } else {
+ this.uploadConfigDir(Paths.get(configDirPath), ConfigSetsHandler.DEFAULT_CONFIGSET_NAME);
+ }
+ }
+ }
+
+ /**
+ * Gets the absolute filesystem path of the _default configset to bootstrap from. First tries the
+ * sysprop "solr.default.confdir". If not found, tries to find the _default dir relative to the
+ * sysprop "solr.install.dir". Returns null if not found anywhere.
+ *
+ * @lucene.internal
+ * @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
+ */
+ public static String getDefaultConfigDirPath() {
Review comment:
this is used by bootstrapDefaultConfigSet()
--
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599022758
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java
##########
@@ -301,7 +302,7 @@ static void deleteBackup(BackupRepository repository, URI backupPath, int maxNum
}
static void validateConfigOrThrowSolrException(SolrCloudManager cloudManager, String configName) throws IOException, KeeperException, InterruptedException {
- boolean isValid = cloudManager.getDistribStateManager().hasData(ZkConfigManager.CONFIGS_ZKNODE + "/" + configName);
+ boolean isValid = cloudManager.getDistribStateManager().hasData(ZkConfigSetService.CONFIGS_ZKNODE + "/" + configName);
Review comment:
SolrCloudManager is in solrj. Since CollectionHandlingUtils is in core, I can pass ConfigSetService as an argument instead of SolrCloudManager.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r602656335
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java
##########
@@ -781,6 +785,14 @@ public String getConfig() {
}
}
+ public List<String> getAllConfigsetFiles(String root) throws KeeperException, InterruptedException {
Review comment:
I believe this is called from exactly one place, so you can inline 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.
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
[GitHub] [solr] NazerkeBS commented on pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on pull request #23:
URL: https://github.com/apache/solr/pull/23#issuecomment-805264001
@dsmiley, for BackupManager, I am getting some test failures about file access permission denial.
--
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r602595930
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -351,6 +282,19 @@ public void downloadConfig(String configName, Path dir) throws IOException {
}
}
+ @Override
+ public String getConfigPath(String configName) throws IOException {
Review comment:
it is `/configs/myconf` if `myconf` exists
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r596823379
##########
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:
Actually we always filter dot-files; so I removed this method
----------------------------------------------------------------
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603478037
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -242,8 +243,7 @@ public static void main(String[] args) throws InterruptedException,
}
String confDir = line.getOptionValue(CONFDIR);
String confName = line.getOptionValue(CONFNAME);
- ZkConfigManager configManager = new ZkConfigManager(zkClient);
- configManager.downloadConfigDir(confName, Paths.get(confDir));
+ ZkMaintenanceUtils.downloadFromZK(zkClient, ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, Paths.get(confDir));
Review comment:
earlier created ZkService is only visible for testing purpose which is not the real ZkConfigSetService
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599046410
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerCollectionConfigSetProcessor.java
##########
@@ -88,9 +88,9 @@ private static OverseerMessageHandlerSelector getOverseerMessageHandlerSelector(
Overseer overseer,
OverseerNodePrioritizer overseerNodePrioritizer) {
final OverseerCollectionMessageHandler collMessageHandler = new OverseerCollectionMessageHandler(
- zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer);
+ zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer, overseer.getCoreContainer());
Review comment:
used by OverseerConfigSetMessageHandler
--
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r602595930
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -351,6 +282,19 @@ public void downloadConfig(String configName, Path dir) throws IOException {
}
}
+ @Override
+ public String getConfigPath(String configName) throws IOException {
Review comment:
it is `/configs/myconf` if `myconf` exists
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599225946
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerCollectionConfigSetProcessor.java
##########
@@ -88,9 +88,9 @@ private static OverseerMessageHandlerSelector getOverseerMessageHandlerSelector(
Overseer overseer,
OverseerNodePrioritizer overseerNodePrioritizer) {
final OverseerCollectionMessageHandler collMessageHandler = new OverseerCollectionMessageHandler(
- zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer);
+ zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer, overseer.getCoreContainer());
Review comment:
The particular like I commented on here is not that, it's `OverseerCollectionMessageHandler`
--
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603256195
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -266,13 +271,14 @@ public void downloadConfig(String configName, Path dir) throws IOException {
@Override
public List<String> getAllConfigFiles(String configName) throws IOException {
String zkPath = CONFIGS_ZKNODE + "/" + configName;
- if (!zkPath.startsWith(ZkConfigSetService.CONFIGS_ZKNODE + "/")) {
- throw new IllegalArgumentException("\"" + zkPath + "\" not recognized as a configset path");
- }
try {
- List<String> filePaths = new LinkedList<>();
- for (String filePath : zkClient.getAllConfigsetFiles(zkPath)) {
- filePaths.add(filePath.replaceAll(zkPath + "/", ""));
+ List<String> filePaths = new ArrayList<>();
+ ZkMaintenanceUtils.traverseZkTree(zkClient, zkPath, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, filePaths::add);
+ filePaths.remove(zkPath);
+ for (int i=0; i<filePaths.size(); i++) {
+ String filePath = filePaths.get(i);
+ assert(filePath.startsWith(zkPath + "/"));
+ filePaths.set(i, filePath.replaceAll(zkPath + "/", ""));
Review comment:
yes, the results start with zkPath (=/configs/configName)
traverseZkTree includes this zkPath (I debugged)
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on pull request #23:
URL: https://github.com/apache/solr/pull/23#issuecomment-801506003
@dsmiley , `TestConfigsApi` class is failing due to NPE from `OverseerConfigSetMessageHandler` where configSetService is null.
----------------------------------------------------------------
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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by 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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r604371316
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -57,7 +57,7 @@ public static ConfigSetService createConfigSetService(CoreContainer coreContaine
final ConfigSetService configSetService = instantiate(coreContainer);
try {
bootstrapDefaultConfigSet(configSetService);
- } catch (IOException e) {
+ } catch (UnsupportedOperationException | IOException e) {
Review comment:
Hmm. Let's catch just UnsupportedOperation. If anything else is thrown; let it propagate to the caller (thus declare that this method throws IOException). I can see in the current code (not this PR), this is what would happen -- it's not swallowed. (not propagating an exception is called swallowing 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.
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599045398
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerCollectionConfigSetProcessor.java
##########
@@ -88,9 +88,9 @@ private static OverseerMessageHandlerSelector getOverseerMessageHandlerSelector(
Overseer overseer,
OverseerNodePrioritizer overseerNodePrioritizer) {
final OverseerCollectionMessageHandler collMessageHandler = new OverseerCollectionMessageHandler(
- zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer);
+ zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer, overseer.getCoreContainer());
final OverseerConfigSetMessageHandler configMessageHandler = new OverseerConfigSetMessageHandler(
- zkStateReader);
+ zkStateReader, overseer.getCoreContainer());
Review comment:
ConfigSetService is instantiated later
--
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
[GitHub] [solr] NazerkeBS commented on pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on pull request #23:
URL: https://github.com/apache/solr/pull/23#issuecomment-801812098
> It's probable the ConfigSetService needs to be registered sooner then.
It seems I have to pass `CoreContainer` instead of `ConfigSetService` not only to `OverseerConfigSetMessageHandler` but also to `OverseerCollectionMessageHandler` which are instantiated in `OverseerMessageHandlerSelector`
----------------------------------------------------------------
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r596736295
##########
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:
I added `getCoreContainer()` to MiniCloudServer
----------------------------------------------------------------
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603905694
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -242,8 +243,7 @@ public static void main(String[] args) throws InterruptedException,
}
String confDir = line.getOptionValue(CONFDIR);
String confName = line.getOptionValue(CONFNAME);
- ZkConfigManager configManager = new ZkConfigManager(zkClient);
- configManager.downloadConfigDir(confName, Paths.get(confDir));
+ ZkMaintenanceUtils.downloadFromZK(zkClient, ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, Paths.get(confDir));
Review comment:
In this case, I have to overload uploadConfig method to add excludePattern param in ConfigSetService
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603237327
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -266,13 +271,14 @@ public void downloadConfig(String configName, Path dir) throws IOException {
@Override
public List<String> getAllConfigFiles(String configName) throws IOException {
String zkPath = CONFIGS_ZKNODE + "/" + configName;
- if (!zkPath.startsWith(ZkConfigSetService.CONFIGS_ZKNODE + "/")) {
- throw new IllegalArgumentException("\"" + zkPath + "\" not recognized as a configset path");
- }
try {
- List<String> filePaths = new LinkedList<>();
- for (String filePath : zkClient.getAllConfigsetFiles(zkPath)) {
- filePaths.add(filePath.replaceAll(zkPath + "/", ""));
+ List<String> filePaths = new ArrayList<>();
+ ZkMaintenanceUtils.traverseZkTree(zkClient, zkPath, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, filePaths::add);
+ filePaths.remove(zkPath);
+ for (int i=0; i<filePaths.size(); i++) {
+ String filePath = filePaths.get(i);
+ assert(filePath.startsWith(zkPath + "/"));
+ filePaths.set(i, filePath.replaceAll(zkPath + "/", ""));
Review comment:
Don't use String.replaceAll when you are looking for something at the beginning or end. replaceAll has to do more work than is needed and looks for other occurrences when it shouldn't have to here. I know calling replaceAll is "easy" on the caller (you) but in Solr we pay more attention to performance.
Given that assert statement, it seems you are confident the results start with zkPath; yes? (tests pass?). I think traverseZkTree ought to not return paths that start with is parameter; it seems wrong to me.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -328,25 +326,26 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
- * Get the config metadata
+ * Get the config metadata (mutable) or null
Review comment:
Instead of null, let's never return null? This would be friendlier on the caller. It would only be not okay if somehow it was meaningful to have blank metadata, which seems weird to me.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r602575524
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -295,9 +248,9 @@ public void downloadConfig(String configName, Path dir) throws IOException {
}
@Override
- public byte[] downloadFileFromConfig(String configName, String fileName) throws IOException {
+ public byte[] downloadFileFromConfig(String filePath) throws IOException {
Review comment:
Surely we need the configName!
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -353,86 +323,55 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void deleteConfig(String configName) throws IOException;
/**
- * Delete a file in config
+ * Delete files in config
*
- * @param configName the config to delete
- * @param fileName the file to delete
- * @throws IOException if an I/O error occurs
- */
- public abstract void deleteFileFromConfig(String configName, String fileName) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
- * @throws IOException if an I/O error occurs
- */
- public abstract void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
+ * @param filesToDelete a list of file paths to delete
* @throws IOException if an I/O error occurs
*/
- public abstract void setConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
/**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
- * @throws IOException if an I/O error occurs or if config does not exist
- */
- public abstract void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
+ * Set the config metadata
+ * If config does not exist, it will be created and set metadata on it
+ * Else metadata will be updated
+ * @param configName the config name
+ * @param data the metadata to be set on config
* @throws IOException if an I/O error occurs
*/
- public abstract void updateConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
- * Get config metadata
+ * Get the config metadata
*
- * @param configName the config
+ * @param configName the config name
* @return the config metadata
* @throws IOException if an I/O error occurs or config does not exist
*/
- public abstract byte[] getConfigMetadata(String configName) throws IOException;
+ public abstract Map<String, Object> getConfigMetadata(String configName) throws IOException;
/**
- * List configs but not intermediate files
+ * List the names of configs
*
- * @return list of configs
+ * @return list the names of configs
* @throws IOException if an I/O error occurs
*/
public abstract List<String> listConfigs() throws IOException;
/**
- * List files in a given config
- *
- * @param configName the config
- * @return list of configs
+ * Get all files in config
+ * @param configName the config name
+ * @return list of file paths under configName excluding itself
Review comment:
```suggestion
* @return list of file name paths in the config
```
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -249,32 +249,23 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
/**
* Upload files from a given path to config
*
- * @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 uploadConfig(Path dir, String configName) throws IOException;
-
- /**
- * Upload a file to config
- *
- * @param configName the name to give the config
- * @param fileName the name of the file
- * @param data the content of the file
+ * @param configName the config name
+ * @param dir {@link Path} to the files
* @throws IOException if an I/O error occurs or the path does not exist
*/
- public abstract void uploadFileToConfig(String configName, String fileName, byte[] data) throws IOException;
+ public abstract void uploadConfig(String configName, Path dir) throws IOException;
/**
* Upload a file to config
- *
+ * If file does not exist, it will be uploaded
+ * If createNew param is set to true then file be overwritten
* @param configName the name to give the config
* @param fileName the name of the file
* @param data the content of the file
- * @param failOnExists if file already exists in config then do not upload
+ * @param overwriteOnExists if true then file will be overwritten
* @throws IOException if an I/O error occurs or the path does not exist
Review comment:
> or the path does not exist
Do you mean, if a file by this name is already in the config? But isn't that typical -- would not yield an exception? I expect maybe the reverse -- if the file already exists, and if overwirteOnExists==false, *then* we throw? I think we should be more specific as to what exception is thrown if the file already exists. This enables the caller to catch it specifically if they need to. For example maybe throw `FileAlreadyExistsException` even though the ConfigSetService isn't necessarily using the file system.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -855,7 +855,11 @@ public static void createClusterZkNodes(SolrZkClient zkClient)
cmdExecutor.ensureExists(ZkStateReader.COLLECTIONS_ZKNODE, zkClient);
cmdExecutor.ensureExists(ZkStateReader.ALIASES, zkClient);
byte[] emptyJson = "{}".getBytes(StandardCharsets.UTF_8);
- cmdExecutor.ensureExists(ZkStateReader.SOLR_SECURITY_CONF_PATH, emptyJson, CreateMode.PERSISTENT, zkClient);
+ cmdExecutor.ensureExists(ZkStateReader.SOLR_SECURITY_CONF_PATH, emptyJson, zkClient);
Review comment:
I'm unsure why you made changes here
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -353,86 +323,55 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void deleteConfig(String configName) throws IOException;
/**
- * Delete a file in config
+ * Delete files in config
*
- * @param configName the config to delete
- * @param fileName the file to delete
- * @throws IOException if an I/O error occurs
- */
- public abstract void deleteFileFromConfig(String configName, String fileName) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
- * @throws IOException if an I/O error occurs
- */
- public abstract void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
+ * @param filesToDelete a list of file paths to delete
* @throws IOException if an I/O error occurs
*/
- public abstract void setConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
/**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
- * @throws IOException if an I/O error occurs or if config does not exist
- */
- public abstract void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
+ * Set the config metadata
+ * If config does not exist, it will be created and set metadata on it
+ * Else metadata will be updated
+ * @param configName the config name
+ * @param data the metadata to be set on config
* @throws IOException if an I/O error occurs
*/
- public abstract void updateConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
- * Get config metadata
+ * Get the config metadata
*
- * @param configName the config
+ * @param configName the config name
* @return the config metadata
* @throws IOException if an I/O error occurs or config does not exist
*/
- public abstract byte[] getConfigMetadata(String configName) throws IOException;
+ public abstract Map<String, Object> getConfigMetadata(String configName) throws IOException;
/**
- * List configs but not intermediate files
+ * List the names of configs
*
- * @return list of configs
+ * @return list the names of configs
* @throws IOException if an I/O error occurs
*/
public abstract List<String> listConfigs() throws IOException;
/**
- * List files in a given config
- *
- * @param configName the config
- * @return list of configs
+ * Get all files in config
+ * @param configName the config name
+ * @return list of file paths under configName excluding itself
* @throws IOException if an I/O error occurs
*/
- public abstract List<String> listFilesInConfig(String configName) throws IOException;
+ public abstract List<String> getAllConfigFiles(String configName) throws IOException;
/**
- * List files in a given config
- *
- * @param configName the config
- * @return list of configs
- * @throws IOException if an I/O error occurs
+ * Get config path
Review comment:
Again, I'm confused by this method. Despite all this documentation.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -351,6 +282,19 @@ public void downloadConfig(String configName, Path dir) throws IOException {
}
}
+ @Override
+ public String getConfigPath(String configName) throws IOException {
Review comment:
This method confuses me. What does the path to a config mean?
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -288,12 +279,11 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
/**
* Download a file from config
*
- * @param configName the config to download
- * @param fileName the file to download
+ * @param filePath the file to download
* @return the content of the file
* @throws IOException if an I/O error occurs or the config does not exist
Review comment:
If we try to do anything to a config and the config doesn't exist, we shouldn't throw an IO Exception IMO. That's not I/O. It may be a lot of work to make this change so for now I would just not document that we behave in that way. We needn't have javadocs for a given aspect (e.g. throwing IOException) if we don't have anything to say about it that's worth the reader's time to read it and your time to write it. I think this is one of those cases.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -180,12 +171,13 @@ public void deleteConfig(String configName) throws IOException {
}
@Override
- public void deleteFileFromConfig(String configName, String fileName) throws IOException {
+ public void deleteFilesFromConfig(List<String> filesToDelete) throws IOException {
Review comment:
Uh... surely we need the configName!
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -315,35 +305,15 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
*/
public abstract void copyConfig(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException;
Review comment:
The parameter "copiedToZkPaths" is obviously misnamed because of the "Zk". But moreover, it seems like a strange parameter to have; I think we should get rid of it. If the caller wants to know, it could list the config files of fromConfig that are there before copying; right?
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -249,32 +249,23 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
/**
* Upload files from a given path to config
*
- * @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 uploadConfig(Path dir, String configName) throws IOException;
-
- /**
- * Upload a file to config
- *
- * @param configName the name to give the config
- * @param fileName the name of the file
- * @param data the content of the file
+ * @param configName the config name
+ * @param dir {@link Path} to the files
* @throws IOException if an I/O error occurs or the path does not exist
*/
- public abstract void uploadFileToConfig(String configName, String fileName, byte[] data) throws IOException;
+ public abstract void uploadConfig(String configName, Path dir) throws IOException;
/**
* Upload a file to config
- *
+ * If file does not exist, it will be uploaded
+ * If createNew param is set to true then file be overwritten
* @param configName the name to give the config
* @param fileName the name of the file
* @param data the content of the file
- * @param failOnExists if file already exists in config then do not upload
+ * @param overwriteOnExists if true then file will be overwritten
* @throws IOException if an I/O error occurs or the path does not exist
*/
- public abstract void uploadFileToConfig(String configName, String fileName, byte[] data, boolean failOnExists) throws IOException;
+ public abstract void uploadFileToConfig(String configName, String fileName, byte[] data, boolean overwriteOnExists) throws IOException;
/**
* Download from config and write it to the filesystem
Review comment:
```suggestion
* Download all files from this config to the filesystem at {@code dir}
```
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -353,86 +323,55 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void deleteConfig(String configName) throws IOException;
/**
- * Delete a file in config
+ * Delete files in config
*
- * @param configName the config to delete
- * @param fileName the file to delete
- * @throws IOException if an I/O error occurs
- */
- public abstract void deleteFileFromConfig(String configName, String fileName) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
- * @throws IOException if an I/O error occurs
- */
- public abstract void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
+ * @param filesToDelete a list of file paths to delete
* @throws IOException if an I/O error occurs
*/
- public abstract void setConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
/**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
- * @throws IOException if an I/O error occurs or if config does not exist
- */
- public abstract void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
+ * Set the config metadata
+ * If config does not exist, it will be created and set metadata on it
Review comment:
Wow; this creates a config. That's unexpected I think, given its name. But I can sorta see how this is and maybe all the methods on this class that upload data will, in effect, create a config by virtue of setting data. Ok.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -353,86 +323,55 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void deleteConfig(String configName) throws IOException;
/**
- * Delete a file in config
+ * Delete files in config
*
- * @param configName the config to delete
- * @param fileName the file to delete
- * @throws IOException if an I/O error occurs
- */
- public abstract void deleteFileFromConfig(String configName, String fileName) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
- * @throws IOException if an I/O error occurs
- */
- public abstract void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
+ * @param filesToDelete a list of file paths to delete
* @throws IOException if an I/O error occurs
*/
- public abstract void setConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
/**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
- * @throws IOException if an I/O error occurs or if config does not exist
- */
- public abstract void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
+ * Set the config metadata
+ * If config does not exist, it will be created and set metadata on it
+ * Else metadata will be updated
Review comment:
```suggestion
* Else metadata will be replaced with the provided metadata.
```
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -353,86 +323,55 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void deleteConfig(String configName) throws IOException;
/**
- * Delete a file in config
+ * Delete files in config
*
- * @param configName the config to delete
- * @param fileName the file to delete
- * @throws IOException if an I/O error occurs
- */
- public abstract void deleteFileFromConfig(String configName, String fileName) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
- * @throws IOException if an I/O error occurs
- */
- public abstract void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
+ * @param filesToDelete a list of file paths to delete
* @throws IOException if an I/O error occurs
*/
- public abstract void setConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
/**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
- * @throws IOException if an I/O error occurs or if config does not exist
- */
- public abstract void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
+ * Set the config metadata
+ * If config does not exist, it will be created and set metadata on it
+ * Else metadata will be updated
+ * @param configName the config name
+ * @param data the metadata to be set on config
* @throws IOException if an I/O error occurs
*/
- public abstract void updateConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
- * Get config metadata
+ * Get the config metadata
*
- * @param configName the config
+ * @param configName the config name
* @return the config metadata
* @throws IOException if an I/O error occurs or config does not exist
*/
- public abstract byte[] getConfigMetadata(String configName) throws IOException;
+ public abstract Map<String, Object> getConfigMetadata(String configName) throws IOException;
/**
- * List configs but not intermediate files
+ * List the names of configs
*
- * @return list of configs
+ * @return list the names of configs
* @throws IOException if an I/O error occurs
*/
public abstract List<String> listConfigs() throws IOException;
/**
- * List files in a given config
- *
- * @param configName the config
- * @return list of configs
+ * Get all files in config
Review comment:
You can do better than that :-) Firstly, this method returns the *names* of the files, not the files themselves. Then mention the format of the name considering the directly like nature of a configSet. Do file names start with a '/' or not? (I'd rather not)
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -353,86 +323,55 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract void deleteConfig(String configName) throws IOException;
/**
- * Delete a file in config
+ * Delete files in config
*
- * @param configName the config to delete
- * @param fileName the file to delete
- * @throws IOException if an I/O error occurs
- */
- public abstract void deleteFileFromConfig(String configName, String fileName) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
- * @throws IOException if an I/O error occurs
- */
- public abstract void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Set config metadata
- *
- * @param configName the config
- * @param data metadata to be set in config
+ * @param filesToDelete a list of file paths to delete
* @throws IOException if an I/O error occurs
*/
- public abstract void setConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
/**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
- * @throws IOException if an I/O error occurs or if config does not exist
- */
- public abstract void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException;
-
- /**
- * Update config metadata
- *
- * @param configName the config
- * @param data metadata to be updated in config
+ * Set the config metadata
+ * If config does not exist, it will be created and set metadata on it
+ * Else metadata will be updated
+ * @param configName the config name
+ * @param data the metadata to be set on config
* @throws IOException if an I/O error occurs
*/
- public abstract void updateConfigMetadata(String configName, byte[] data) throws IOException;
+ public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
- * Get config metadata
+ * Get the config metadata
*
- * @param configName the config
+ * @param configName the config name
* @return the config metadata
Review comment:
Does it return null? It'd be nice on callers if we can guarantee non-null. Also mention wether the result is mutable.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599048184
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java
##########
@@ -67,6 +68,7 @@
HttpShardHandlerFactory shardHandlerFactory;
String adminPath;
ZkStateReader zkStateReader;
+ CoreContainer cc;
Review comment:
I passed it; OverseerMessageHandlerSelector creates an instance of OverseerCollectionMessageHandler and OverseerConfigSetMessageHandler and returns it as a OverseerMessageHandlerSelector.
--
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r596823131
##########
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 would prefer to rename to `deleteConfig`
----------------------------------------------------------------
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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r604372030
##########
File path: solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java
##########
@@ -62,7 +60,13 @@ public String configSetName(CoreDescriptor cd) {
@Override
public boolean checkConfigExists(String configName) throws IOException {
- return true;
+ List<String> configs = listConfigs();
+ for (String config : configs) {
Review comment:
Why call endsWith?
listConfigs should return a simple list of config names. If it doesn't (if it returns path info with slashes) then it's broken and should be fixed. Making the change here as you did works around a root cause; always fix root causes instead.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #23:
URL: https://github.com/apache/solr/pull/23#issuecomment-801565991
It's probable the ConfigSetService needs to be registered sooner then.
----------------------------------------------------------------
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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r601875746
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -191,19 +213,79 @@ public void uploadConfig(Path dir, String configName) throws IOException {
}
@Override
- public void uploadFileToConfig(String fileName, String configName) throws IOException {
- Path path = Paths.get(fileName);
- if (!Files.exists(path)) {
- throw new IOException("File path " + path + " does not exist");
+ public void createFilePathInConfig(String configName, String fileName, boolean failOnExists) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, failOnExists,true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file path in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data, boolean failOnExists) throws IOException {
+ String filePath = CONFIGS_ZKNODE + "/" + configName + "/" + fileName;
+ try {
+ zkClient.makePath(filePath, data, CreateMode.PERSISTENT, null, failOnExists, true);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The path " + filePath + " for configSet " + configName + " already exists. In order to overwrite, provide overwrite=true or use an HTTP PUT with the V2 API.");
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName, Utils.toJSON(data), true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error setting config metadata", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, byte[] data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error setting config metadata", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException {
+ try {
+ zkClient.setData(CONFIGS_ZKNODE + "/" + configName, Utils.toJSON(data), true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error updating config metadata", SolrZkClient.checkInterrupted(e));
}
- File file = path.toFile();
- String zkNode = CONFIGS_ZKNODE + "/" + configName + "/" + file.getName();
+ }
+
+ @Override
+ public void updateConfigMetadata(String configName, byte[] data) throws IOException {
+ try {
+ zkClient.setData(CONFIGS_ZKNODE + "/" + configName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error updating config metadata", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public byte[] getConfigMetadata(String configName) throws IOException {
Review comment:
Lets work with Maps and not byte arrays for metadata. Bytes are too low level.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -191,19 +213,79 @@ public void uploadConfig(Path dir, String configName) throws IOException {
}
@Override
- public void uploadFileToConfig(String fileName, String configName) throws IOException {
- Path path = Paths.get(fileName);
- if (!Files.exists(path)) {
- throw new IOException("File path " + path + " does not exist");
+ public void createFilePathInConfig(String configName, String fileName, boolean failOnExists) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, failOnExists,true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file path in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data, boolean failOnExists) throws IOException {
+ String filePath = CONFIGS_ZKNODE + "/" + configName + "/" + fileName;
+ try {
+ zkClient.makePath(filePath, data, CreateMode.PERSISTENT, null, failOnExists, true);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The path " + filePath + " for configSet " + configName + " already exists. In order to overwrite, provide overwrite=true or use an HTTP PUT with the V2 API.");
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName, Utils.toJSON(data), true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error setting config metadata", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, byte[] data) throws IOException {
Review comment:
No; I think the API of ConfigSetService should keep this higher level than bytes. The Map one is perfect.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -246,23 +247,53 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract String configSetName(CoreDescriptor cd);
/**
- * Check whether a config exists
+ * Upload files from a given path to config
*
- * @param configName the config to check existance on
- * @return whether the config exists or not
- * @throws IOException if an I/O error occurs
+ * @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 boolean checkConfigExists(String configName) throws IOException {
- return listConfigs().contains(configName);
- }
+ public abstract void uploadConfig(Path dir, String configName) throws IOException;
Review comment:
Let's have a consistent/purposeful approach to parameter ordering in this class. We could make "configName" first *always*. Or, we could vary it to go last *only* when we uploading (e.g. imagine an arrow showing the direction of the data from first param to second).
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java
##########
@@ -781,6 +785,14 @@ public String getConfig() {
}
}
+ public List<String> getAllConfigsetFiles(String root) throws KeeperException, InterruptedException {
Review comment:
This is needed on SolrZkClient for some reason?
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -191,19 +213,79 @@ public void uploadConfig(Path dir, String configName) throws IOException {
}
@Override
- public void uploadFileToConfig(String fileName, String configName) throws IOException {
- Path path = Paths.get(fileName);
- if (!Files.exists(path)) {
- throw new IOException("File path " + path + " does not exist");
+ public void createFilePathInConfig(String configName, String fileName, boolean failOnExists) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, failOnExists,true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file path in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data, boolean failOnExists) throws IOException {
Review comment:
RE "failOnExists": Lets standardize on naming used by Files.write (see StandardOpenOption) -- "CREATE_NEW"
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -285,27 +316,124 @@ public boolean checkConfigExists(String configName) throws IOException {
public abstract void copyConfig(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException;
/**
- * Upload files from a given path to a config Zookeeper
+ * Create file path in config
*
- * @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
+ * @param configName the config to download
+ * @param fileName file path to be created
+ * @param failOnExists if file path already exists then do not re-create
+ * @throws IOException if an I/O error occurs
*/
- public abstract void uploadConfig(Path dir, String configName) throws IOException;
+ public abstract void createFilePathInConfig(String configName, String fileName, boolean failOnExists) throws IOException;
Review comment:
I'm skeptical this is necessary. Instead, files can be uploaded that have slashes in there and we can create recursively as needed.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -223,6 +315,42 @@ public void downloadConfig(String configName, Path dir) throws IOException {
}
}
+ @Override
+ public List<String> listFilesInConfig(String configName) throws IOException {
+ try {
+ return zkClient.getChildren(CONFIGS_ZKNODE + "/" + configName, null, true);
+ } catch (KeeperException.NoNodeException e) {
+ return Collections.emptyList();
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException(e);
+ }
+ }
+
+ @Override
+ public List<String> listFilesInConfig(String configName, String fileName) throws IOException {
Review comment:
You added getAllConfigsetFiles which seems to obsolete this method? Though I like the name of this one better.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -191,19 +213,79 @@ public void uploadConfig(Path dir, String configName) throws IOException {
}
@Override
- public void uploadFileToConfig(String fileName, String configName) throws IOException {
- Path path = Paths.get(fileName);
- if (!Files.exists(path)) {
- throw new IOException("File path " + path + " does not exist");
+ public void createFilePathInConfig(String configName, String fileName, boolean failOnExists) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, failOnExists,true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file path in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName + "/" + fileName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data, boolean failOnExists) throws IOException {
+ String filePath = CONFIGS_ZKNODE + "/" + configName + "/" + fileName;
+ try {
+ zkClient.makePath(filePath, data, CreateMode.PERSISTENT, null, failOnExists, true);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "The path " + filePath + " for configSet " + configName + " already exists. In order to overwrite, provide overwrite=true or use an HTTP PUT with the V2 API.");
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error creating file in config", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, Map<Object, Object> data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName, Utils.toJSON(data), true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error setting config metadata", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, byte[] data) throws IOException {
+ try {
+ zkClient.makePath(CONFIGS_ZKNODE + "/" + configName, data, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error setting config metadata", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void updateConfigMetadata(String configName, Map<Object, Object> data) throws IOException {
Review comment:
I observe you added setConfigMetadata yet also have updateConfigMetadata. They are the same! (a bug surely). I suspect we may not need the "update" version.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603910002
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
including its caller `bootstrapDefaultConfigSet `
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603502495
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -242,8 +243,7 @@ public static void main(String[] args) throws InterruptedException,
}
String confDir = line.getOptionValue(CONFDIR);
String confName = line.getOptionValue(CONFNAME);
- ZkConfigManager configManager = new ZkConfigManager(zkClient);
- configManager.downloadConfigDir(confName, Paths.get(confDir));
+ ZkMaintenanceUtils.downloadFromZK(zkClient, ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, Paths.get(confDir));
Review comment:
I'm looking at the file now and I'm confused and what you said. I am proposing that ZkCLI's main method declare a ZkConfigSetService that can be used by several of the operations executed.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603243000
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerConfigSetMessageHandler.java
##########
@@ -340,7 +320,7 @@ private void createConfigSet(ZkNodeProps message) throws IOException {
// the entire baseConfig set with the old properties, including immutable,
// that would make it impossible for the user to delete.
try {
- if (configManager.configExists(configSetName) && copiedToZkPaths.size() > 0) {
+ if (coreContainer.getConfigSetService().checkConfigExists(configSetName) && coreContainer.getConfigSetService().getAllConfigFiles(configSetName).size() > 0) {
Review comment:
If you look at the surrounding context of when this occurs, I think you need not check to see that the configSet we are considering has files or not. Just delete it.
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
`getDefaultConfigDirPath` seems not specific to ZK?
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -296,51 +293,41 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
- private void downloadFromZK(SolrZkClient zkClient, String zkPath, URI dir) throws IOException {
- try {
- List<String> files = zkClient.getChildren(zkPath, null, true);
- for (String file : files) {
- List<String> children = zkClient.getChildren(zkPath + "/" + file, null, true);
- if (children.size() == 0) {
- log.debug("Writing file {}", file);
- byte[] data = zkClient.getData(zkPath + "/" + file, null, null, true);
- try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
- os.write(data);
- }
- } else {
- URI uri = repository.resolve(dir, file);
- if (!repository.exists(uri)) {
- repository.createDirectory(uri);
- }
- downloadFromZK(zkClient, zkPath + "/" + file, repository.resolve(dir, file));
+ private void downloadConfig(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> filePaths = configSetService.getAllConfigFiles(configName);
+ for (String filePath : filePaths) {
+ URI uri = repository.resolve(dir, filePath);
+ if (!filePath.endsWith("/")) {
+ log.debug("Writing file {}", filePath);
+ byte[] data = configSetService.downloadFileFromConfig(configName, filePath);
+ try (OutputStream os = repository.createOutput(uri)) {
+ os.write(data);
}
+ } else {
+ if (!repository.exists(uri)) {
+ repository.createDirectory(uri);
+ }
+ downloadConfig(configSetService, configName, uri);
}
- } catch (KeeperException | InterruptedException e) {
- throw new IOException("Error downloading files from zookeeper path " + zkPath + " to " + dir.toString(),
- SolrZkClient.checkInterrupted(e));
}
}
- private void uploadToZk(SolrZkClient zkClient, URI sourceDir, String destZkPath) throws IOException {
+ private void uploadConfig(ConfigSetService configSetService, URI sourceDir, String configName) throws IOException {
Review comment:
Consider naming `uploadConfigFromRepo`
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -296,51 +293,41 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
- private void downloadFromZK(SolrZkClient zkClient, String zkPath, URI dir) throws IOException {
- try {
- List<String> files = zkClient.getChildren(zkPath, null, true);
- for (String file : files) {
- List<String> children = zkClient.getChildren(zkPath + "/" + file, null, true);
- if (children.size() == 0) {
- log.debug("Writing file {}", file);
- byte[] data = zkClient.getData(zkPath + "/" + file, null, null, true);
- try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
- os.write(data);
- }
- } else {
- URI uri = repository.resolve(dir, file);
- if (!repository.exists(uri)) {
- repository.createDirectory(uri);
- }
- downloadFromZK(zkClient, zkPath + "/" + file, repository.resolve(dir, file));
+ private void downloadConfig(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> filePaths = configSetService.getAllConfigFiles(configName);
+ for (String filePath : filePaths) {
+ URI uri = repository.resolve(dir, filePath);
+ if (!filePath.endsWith("/")) {
+ log.debug("Writing file {}", filePath);
+ byte[] data = configSetService.downloadFileFromConfig(configName, filePath);
+ try (OutputStream os = repository.createOutput(uri)) {
+ os.write(data);
}
+ } else {
+ if (!repository.exists(uri)) {
+ repository.createDirectory(uri);
+ }
+ downloadConfig(configSetService, configName, uri);
}
- } catch (KeeperException | InterruptedException e) {
- throw new IOException("Error downloading files from zookeeper path " + zkPath + " to " + dir.toString(),
- SolrZkClient.checkInterrupted(e));
}
}
- private void uploadToZk(SolrZkClient zkClient, URI sourceDir, String destZkPath) throws IOException {
+ private void uploadConfig(ConfigSetService configSetService, URI sourceDir, String configName) throws IOException {
for (String file : repository.listAll(sourceDir)) {
- String zkNodePath = destZkPath + "/" + file;
URI path = repository.resolve(sourceDir, file);
- PathType t = repository.getPathType(path);
+ BackupRepository.PathType t = repository.getPathType(path);
switch (t) {
case FILE: {
try (IndexInput is = repository.openInput(sourceDir, file, IOContext.DEFAULT)) {
byte[] arr = new byte[(int) is.length()]; // probably ok since the config file should be small.
is.readBytes(arr, 0, (int) is.length());
- zkClient.makePath(zkNodePath, arr, true);
- } catch (KeeperException | InterruptedException e) {
- throw new IOException(SolrZkClient.checkInterrupted(e));
+ configSetService.uploadFileToConfig(configName, file, arr, false);
}
break;
}
-
case DIRECTORY: {
if (!file.startsWith(".")) {
- uploadToZk(zkClient, path, zkNodePath);
+ uploadConfig(configSetService, path, configName);
Review comment:
Again; I find this suspicious and probably wrong. Before it was fully recursive and I can see that it was correct because it kept adjusting zkNodePath and path. For it to work with this new path (still recursion), you would need a new param that is the prefix of the file name to upload into the configSet.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/RestoreCmd.java
##########
@@ -274,13 +274,15 @@ private void validate(DocCollection backupCollectionState, int availableNodeCoun
assert totalReplicasPerShard > 0;
}
- private void uploadConfig(String configName, String restoreConfigName, ZkStateReader zkStateReader, BackupManager backupMgr) throws IOException {
- if (zkStateReader.getConfigManager().configExists(restoreConfigName)) {
+ private void uploadConfig(String configName, String restoreConfigName, BackupManager backupMgr, CoreContainer container) throws IOException {
Review comment:
Looks like you should just pass the ConfigSetService here instead of CoreContainer
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -889,48 +858,10 @@ public static void createClusterZkNodes(SolrZkClient zkClient)
cmdExecutor.ensureExists(ZkStateReader.SOLR_SECURITY_CONF_PATH, emptyJson, zkClient);
if (zkClient.getACL(ZkStateReader.SOLR_SECURITY_CONF_PATH, null, true).equals(OPEN_ACL_UNSAFE)) {
log.warn("Contents of zookeeper /security.json are world-readable;" +
- " consider setting up ACLs as described in https://solr.apache.org/guide/zookeeper-access-control.html");
- }
- bootstrapDefaultConfigSet(zkClient);
Review comment:
You removed this call to bootstrap the default ConfigSet here. Does it happen somewhere else?
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -71,7 +72,7 @@
static final String UPCONFIG = "upconfig";
static final String EXCLUDE_REGEX_SHORT = "x";
static final String EXCLUDE_REGEX = "excluderegex";
- static final String EXCLUDE_REGEX_DEFAULT = ZkConfigManager.UPLOAD_FILENAME_EXCLUDE_REGEX;
+ static final String EXCLUDE_REGEX_DEFAULT = ZkConfigSetService.UPLOAD_FILENAME_EXCLUDE_REGEX;
Review comment:
This constant ought to be defined in ConfigSetService as it's not intrinsically ZK specific.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -242,8 +243,7 @@ public static void main(String[] args) throws InterruptedException,
}
String confDir = line.getOptionValue(CONFDIR);
String confName = line.getOptionValue(CONFNAME);
- ZkConfigManager configManager = new ZkConfigManager(zkClient);
- configManager.downloadConfigDir(confName, Paths.get(confDir));
+ ZkMaintenanceUtils.downloadFromZK(zkClient, ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, Paths.get(confDir));
Review comment:
Given that earlier to created a ZkConfigSetService and saved it on CoreContainer, can't you use that?
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -296,51 +293,41 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
- private void downloadFromZK(SolrZkClient zkClient, String zkPath, URI dir) throws IOException {
- try {
- List<String> files = zkClient.getChildren(zkPath, null, true);
- for (String file : files) {
- List<String> children = zkClient.getChildren(zkPath + "/" + file, null, true);
- if (children.size() == 0) {
- log.debug("Writing file {}", file);
- byte[] data = zkClient.getData(zkPath + "/" + file, null, null, true);
- try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
- os.write(data);
- }
- } else {
- URI uri = repository.resolve(dir, file);
- if (!repository.exists(uri)) {
- repository.createDirectory(uri);
- }
- downloadFromZK(zkClient, zkPath + "/" + file, repository.resolve(dir, file));
+ private void downloadConfig(ConfigSetService configSetService, String configName, URI dir) throws IOException {
Review comment:
upload vs download is ambiguous when we're dealing with two services (BackupRepository & ConfigSetService) so the method name here should add clarity. I suggest `downloadConfigToRepo`
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -231,9 +233,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);
+ ZkMaintenanceUtils.uploadToZK(zkClient, Paths.get(confDir), ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, excludePattern);
Review comment:
Given that earlier to created a ZkConfigSetService and saved it on CoreContainer, can't you use that?
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -296,51 +293,41 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
- private void downloadFromZK(SolrZkClient zkClient, String zkPath, URI dir) throws IOException {
- try {
- List<String> files = zkClient.getChildren(zkPath, null, true);
- for (String file : files) {
- List<String> children = zkClient.getChildren(zkPath + "/" + file, null, true);
- if (children.size() == 0) {
- log.debug("Writing file {}", file);
- byte[] data = zkClient.getData(zkPath + "/" + file, null, null, true);
- try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
- os.write(data);
- }
- } else {
- URI uri = repository.resolve(dir, file);
- if (!repository.exists(uri)) {
- repository.createDirectory(uri);
- }
- downloadFromZK(zkClient, zkPath + "/" + file, repository.resolve(dir, file));
+ private void downloadConfig(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> filePaths = configSetService.getAllConfigFiles(configName);
+ for (String filePath : filePaths) {
+ URI uri = repository.resolve(dir, filePath);
+ if (!filePath.endsWith("/")) {
+ log.debug("Writing file {}", filePath);
+ byte[] data = configSetService.downloadFileFromConfig(configName, filePath);
+ try (OutputStream os = repository.createOutput(uri)) {
+ os.write(data);
}
+ } else {
+ if (!repository.exists(uri)) {
+ repository.createDirectory(uri);
+ }
+ downloadConfig(configSetService, configName, uri);
Review comment:
this looks suspicious to me. It used to work recursively (before your changes) -- it called getChildren which I think was only one level deep. But calling getAllConfigFiles means we get full depth, and thus shouldn't be using recursion here. Looks like this oversight would have been a nasty bug
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r604034499
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -344,8 +344,9 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
public abstract List<String> listConfigs() throws IOException;
/**
- * Get the names of the files in config including dir (mutable, non-null)
- * e.g. solrconfig.xml, lang/stopwords_en.txt, lang/stoptags_en.txt
+ * Get the names of the files in config including dirs (mutable, non-null)
+ * Sorted lexicographically
+ * e.g. solrconfig.xml, lang/, lang/stoptags_en.txt, lang/stopwords_en.txt
Review comment:
oh more more little thing. I've never seen a file in Solr called "stoptags_en.txt" and just recently noticed there is one or two "stoptags_ja.txt" so that would better. But it's extra; we only need _one_ file in the example in the subdir, so stopwords_en.txt is common & sufficient without "stoptags" (whatever that is).
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -277,17 +277,22 @@ public void downloadConfig(String configName, Path dir) throws IOException {
ZkMaintenanceUtils.traverseZkTree(zkClient, zkPath, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, filePaths::add);
filePaths.remove(zkPath);
- for (int i=0; i<filePaths.size(); i++) {
- String filePath = filePaths.get(i);
- // if it is a directory, concatenate '/' to the filePath
- if (zkClient.getChildren(filePath, null, true).size() != 0) {
- filePath = filePath + "/";
- }
+ String prevPath = null;
+ for (int i = 0; i < filePaths.size(); i++) {
+ String currPath = filePaths.get(i);
+
// stripping /configs/configName/
- assert(filePath.startsWith(zkPath + "/"));
- filePath = filePath.substring(zkPath.length()+1);
- filePaths.set(i, filePath);
+ assert (currPath.startsWith(zkPath + "/"));
Review comment:
The wrapping parenthesis is weird; please remove them.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -277,17 +277,22 @@ public void downloadConfig(String configName, Path dir) throws IOException {
ZkMaintenanceUtils.traverseZkTree(zkClient, zkPath, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, filePaths::add);
filePaths.remove(zkPath);
- for (int i=0; i<filePaths.size(); i++) {
- String filePath = filePaths.get(i);
- // if it is a directory, concatenate '/' to the filePath
- if (zkClient.getChildren(filePath, null, true).size() != 0) {
- filePath = filePath + "/";
- }
+ String prevPath = null;
+ for (int i = 0; i < filePaths.size(); i++) {
+ String currPath = filePaths.get(i);
+
// stripping /configs/configName/
- assert(filePath.startsWith(zkPath + "/"));
- filePath = filePath.substring(zkPath.length()+1);
- filePaths.set(i, filePath);
+ assert (currPath.startsWith(zkPath + "/"));
+ currPath = currPath.substring(zkPath.length() + 1);
+
+ // if currentPath is a directory, concatenate '/'
+ if (prevPath != null && prevPath.contains(currPath)) {
Review comment:
No, not "contains"; lets be more specific.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -277,17 +277,22 @@ public void downloadConfig(String configName, Path dir) throws IOException {
ZkMaintenanceUtils.traverseZkTree(zkClient, zkPath, ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, filePaths::add);
filePaths.remove(zkPath);
- for (int i=0; i<filePaths.size(); i++) {
- String filePath = filePaths.get(i);
- // if it is a directory, concatenate '/' to the filePath
- if (zkClient.getChildren(filePath, null, true).size() != 0) {
- filePath = filePath + "/";
- }
+ String prevPath = null;
Review comment:
If you use the empty string, it will simplify the below code slightly.
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -323,15 +323,13 @@ private void uploadConfigToConfigRepo(ConfigSetService configSetService, URI sou
is.readBytes(arr, 0, (int) is.length());
if(filePath.startsWith("/")) {
Review comment:
I think you misunderstand what I was trying to say. Can you see how that leading "/" got there in the first place? (hint; it's earlier in this very method). So my advise is to not put it there in the first place (instead of always removing later).
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603647197
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
But it is used in the ZkConfigSetService constructor
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603509754
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
Ah, I see. bootstrapDefaultConfigSet is also not ZK specific so should move there too :-)
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603478725
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -889,48 +858,10 @@ public static void createClusterZkNodes(SolrZkClient zkClient)
cmdExecutor.ensureExists(ZkStateReader.SOLR_SECURITY_CONF_PATH, emptyJson, zkClient);
if (zkClient.getACL(ZkStateReader.SOLR_SECURITY_CONF_PATH, null, true).equals(OPEN_ACL_UNSAFE)) {
log.warn("Contents of zookeeper /security.json are world-readable;" +
- " consider setting up ACLs as described in https://solr.apache.org/guide/zookeeper-access-control.html");
- }
- bootstrapDefaultConfigSet(zkClient);
Review comment:
Yes it is called in ZkConfigSetService constructor
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #23:
URL: https://github.com/apache/solr/pull/23
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603906237
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -231,9 +233,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);
+ ZkMaintenanceUtils.uploadToZK(zkClient, Paths.get(confDir), ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, excludePattern);
Review comment:
In this case, I have to overload uploadConfig method to add excludePattern param in ConfigSetService, is it fine?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r599577580
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -0,0 +1,319 @@
+/*
+ * 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.File;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+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.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.CoreContainer;
+import org.apache.solr.core.CoreDescriptor;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.servlet.SolrDispatchFilter;
+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 final ZkController zkController;
+ private final SolrZkClient zkClient;
+ /** 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(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.zkController = cc.getZkController();
+ this.zkClient = cc.getZkController().getZkClient();
+ try {
+ bootstrapDefaultConfigSet();
+ } catch (IOException e) {
+ log.error("Error in bootstrapping default config");
+ }
+ }
+
+ /** This is for ZkCLI and some tests */
+ public ZkConfigSetService(SolrZkClient zkClient) {
+ super(null, false);
+ this.zkController = null;
+ this.zkClient = zkClient;
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ final String colName = cd.getCollectionName();
+
+ // For back compat with cores that can create collections without the collections API
+ try {
+ if (!zkClient.exists(ZkStateReader.COLLECTIONS_ZKNODE + "/" + colName, true)) {
+ // TODO remove this functionality or maybe move to a CLI mechanism
+ log.warn("Auto-creating collection (in ZK) from core descriptor (on disk). This feature may go away!");
+ CreateCollectionCmd.createCollectionZkNode(zkController.getSolrCloudManager().getDistribStateManager(), colName, cd.getCloudDescriptor().getParams());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Interrupted auto-creating collection", e);
+ } catch (KeeperException e) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Failure auto-creating collection", e);
+ }
+
+ // The configSet is read from ZK and populated. Ignore CD's pre-existing configSet; only populated in standalone
+ final String configSetName;
+ try {
+ configSetName = zkController.getZkStateReader().readConfigName(colName);
+ cd.setConfigSet(configSetName);
+ } catch (KeeperException ex) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Trouble resolving configSet for collection " + colName + ": " + ex.getMessage());
+ }
+
+ return new ZkSolrResourceLoader(cd.getInstanceDir(), configSetName, parentLoader.getClassLoader(), zkController);
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes"})
+ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loader) {
+ try {
+ return ConfigSetProperties.readFromResourceLoader(loader, ".");
+ } catch (Exception ex) {
+ log.debug("No configSet flags", ex);
+ return null;
+ }
+ }
+
+ @Override
+ protected Long getCurrentSchemaModificationVersion(String configSet, SolrConfig solrConfig, String schemaFile) {
+ String zkPath = CONFIGS_ZKNODE + "/" + configSet + "/" + schemaFile;
+ Stat stat;
+ try {
+ stat = zkClient.exists(zkPath, null, true);
+ } catch (KeeperException e) {
+ log.warn("Unexpected exception when getting modification time of {}", zkPath, e);
+ return null; // debatable; we'll see an error soon if there's a real problem
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ if (stat == null) { // not found
+ return null;
+ }
+ return (long) stat.getVersion();
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return "configset " + cd.getConfigSet();
+ }
+
+ @Override
+ public boolean checkConfigExists(String configName) throws IOException {
+ try {
+ return zkClient.exists(CONFIGS_ZKNODE + "/" + configName, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void deleteConfigDir(String configName) throws IOException {
+ try {
+ zkClient.clean(CONFIGS_ZKNODE + "/" + configName);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig) throws IOException {
+ copyConfigDir(fromConfig, toConfig, null);
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException {
+ String fromConfigPath = CONFIGS_ZKNODE + "/" + fromConfig;
+ String toConfigPath = CONFIGS_ZKNODE + "/" + toConfig;
+ try {
+ copyData(copiedToZkPaths, fromConfigPath, toConfigPath);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error config " + fromConfig + " to " + toConfig,
+ SolrZkClient.checkInterrupted(e));
+ }
+ copyConfigDirFromZk(fromConfigPath, toConfigPath, copiedToZkPaths);
+ }
+
+ @Override
+ public void uploadConfigDir(Path dir, String configName) throws IOException {
+ zkClient.uploadToZK(dir, CONFIGS_ZKNODE + "/" + configName, ZkConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN);
+ }
+
+ @Override
+ public void downloadConfigDir(String configName, Path dir) throws IOException {
+ zkClient.downloadFromZK(CONFIGS_ZKNODE + "/" + configName, dir);
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ try {
+ return zkClient.getChildren(CONFIGS_ZKNODE, null, true);
+ } catch (KeeperException.NoNodeException e) {
+ return Collections.emptyList();
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error listing configs", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ private void bootstrapDefaultConfigSet() throws IOException {
Review comment:
We call this method during initialisation of ZkConfigSetService
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r604847464
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -748,7 +748,12 @@ public void load() {
warnUsersOfInsecureSettings();
this.backupRepoFactory = new BackupRepositoryFactory(cfg.getBackupRepositoryPlugins());
- coreConfigService = ConfigSetService.createConfigSetService(this);
+ try {
Review comment:
If you look around at the existing code up & down, you can see that plugins are initialized with a one-liner call without try-catch. So assuming we want to clarify errors coming from createConfigSetService as being related to configSet creation / initialization (Maybe), then it would be better for that logic to go into createConfigSetService. I know I said yesterday to just propagate the IOException but it wasn't evident at that time there was a need to clarify to the caller that createConfigSetService didn't succeed. Based on where IOException is thrown, it's not even an issue with the creation of configSetService, it's the bootstrapping of a default config.
##########
File path: solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java
##########
@@ -30,123 +30,117 @@
import org.slf4j.LoggerFactory;
/**
- * The Solr standalone version of File System ConfigSetService.
+ * Solr Standalone File System ConfigSetService impl.
+ *
* <p>
- * Loads a ConfigSet defined by the core's configSet property,
- * looking for a directory named for the configSet property value underneath
- * a base directory. If no configSet property is set, loads the ConfigSet
- * instead from the core's instance directory.
+ * Loads a ConfigSet defined by the core's configSet property, looking for a directory named for
+ * the configSet property value underneath a base directory. If no configSet property is set, loads
+ * the ConfigSet instead from the core's instance directory.
*/
public class FileSystemConfigSetService extends ConfigSetService {
- private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private final Path configSetBase;
-
- public FileSystemConfigSetService(CoreContainer cc) {
- super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
- this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
- }
-
- @Override
- public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
- Path instanceDir = locateInstanceDir(cd);
- SolrResourceLoader solrResourceLoader = new SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
- return solrResourceLoader;
- }
-
- @Override
- public String configSetName(CoreDescriptor cd) {
- return (cd.getConfigSet() == null ? "instancedir " : "configset ") + locateInstanceDir(cd);
- }
-
- @Override
- public boolean checkConfigExists(String configName) throws IOException {
- List<String> configs = listConfigs();
- for (String config : configs) {
- if (config.endsWith(configName)) {
- return true;
- }
- }
- return false;
- }
-
- @Override
- public void deleteConfig(String configName) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void deleteFilesFromConfig(String configName, List<String> filesToDelete) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- public void copyConfig(String fromConfig, String toConfig) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void uploadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void uploadFileToConfig(String configName, String fileName, byte[] data, boolean overwriteOnExists) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void setConfigMetadata(String configName, Map<String, Object> data) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public Map<String, Object> getConfigMetadata(String configName) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void downloadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public List<String> listConfigs() throws IOException {
- return Files.list(configSetBase)
- .map(Path::toString)
- .collect(Collectors.toList());
- }
-
- @Override
- public byte[] downloadFileFromConfig(String configName, String filePath) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public List<String> getAllConfigFiles(String configName) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- protected Path locateInstanceDir(CoreDescriptor cd) {
- String configSet = cd.getConfigSet();
- if (configSet == null)
- return cd.getInstanceDir();
- Path configSetDirectory = configSetBase.resolve(configSet);
- if (!Files.isDirectory(configSetDirectory))
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
- "Could not load configuration from directory " + configSetDirectory);
- return configSetDirectory;
- }
-
- @Override
- protected Long getCurrentSchemaModificationVersion(String configSet, SolrConfig solrConfig, String schemaFileName) {
- Path schemaFile = solrConfig.getResourceLoader().getConfigPath().resolve(schemaFileName);
- try {
- return Files.getLastModifiedTime(schemaFile).toMillis();
- } catch (FileNotFoundException e) {
- return null; // acceptable
- } catch (IOException e) {
- log.warn("Unexpected exception when getting modification time of {}", schemaFile, e);
- return null; // debatable; we'll see an error soon if there's a real problem
- }
- }
-
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private final Path configSetBase;
+
+ public FileSystemConfigSetService(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ Path instanceDir = locateInstanceDir(cd);
+ SolrResourceLoader solrResourceLoader = new SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
+ return solrResourceLoader;
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return (cd.getConfigSet() == null ? "instancedir " : "configset ") + locateInstanceDir(cd);
+ }
+
+ @Override
+ public boolean checkConfigExists(String configName) throws IOException {
+ Path configSetDirectory = configSetBase.resolve(configName);
+ return Files.isDirectory(configSetDirectory);
+ }
+
+ @Override
+ public void deleteConfig(String configName) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void deleteFilesFromConfig(String configName, List<String> filesToDelete) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ public void copyConfig(String fromConfig, String toConfig) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void uploadConfig(String configName, Path dir) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[] data, boolean overwriteOnExists) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, Map<String, Object> data) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public Map<String, Object> getConfigMetadata(String configName) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void downloadConfig(String configName, Path dir) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ return Files.list(configSetBase)
Review comment:
We have to use try-with-resources here. See https://github.com/apache/solr/blob/dd22ed1a18a6e3d5b6f599c4c665b6374dc9e24d/solr/core/src/java/org/apache/solr/handler/CatStream.java#L231
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r602596784
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -855,7 +855,11 @@ public static void createClusterZkNodes(SolrZkClient zkClient)
cmdExecutor.ensureExists(ZkStateReader.COLLECTIONS_ZKNODE, zkClient);
cmdExecutor.ensureExists(ZkStateReader.ALIASES, zkClient);
byte[] emptyJson = "{}".getBytes(StandardCharsets.UTF_8);
- cmdExecutor.ensureExists(ZkStateReader.SOLR_SECURITY_CONF_PATH, emptyJson, CreateMode.PERSISTENT, zkClient);
+ cmdExecutor.ensureExists(ZkStateReader.SOLR_SECURITY_CONF_PATH, emptyJson, zkClient);
Review comment:
It was due to merge conflict
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603496799
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
I thought it is relevant here as bootstrapDefaultConfigSet() is calling it which is then called in ZkConfigSetService constructor.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603738550
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
That shouldn't prevent you calling it? (make it protected access)
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603909469
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -1216,15 +1212,15 @@ public void testList() throws Exception {
*
* @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
* @see #setDefaultConfigDirSysPropIfNotSet
- * @see ZkController#getDefaultConfigDirPath
+ * @see ZkConfigSetService#getDefaultConfigDirPath
*/
@Test
public void testUserAndTestDefaultConfigsetsAreSame() throws IOException {
final File extPath = new File(ExternalPaths.DEFAULT_CONFIGSET);
assertTrue("_default dir doesn't exist: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.exists());
assertTrue("_default dir isn't a dir: " + ExternalPaths.DEFAULT_CONFIGSET, extPath.isDirectory());
- final String zkBootStrap = ZkController.getDefaultConfigDirPath();
+ final String zkBootStrap = ZkConfigSetService.getDefaultConfigDirPath();
Review comment:
suggesting this `getDefaultConfigDirPath` should move to ConfigSetService?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r601012351
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -287,6 +293,51 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
+ private void downloadFromZK(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> files = configSetService.listFilesInConfig(configName);
Review comment:
I'm guessing listFilesInConfig does only one level? I see you have it overloaded that takes "file". This feels a bit awkward to me... can't we just list all files in the configSet? The code here looks like it might not work if there are multiple intermediate levels (since there is no recursion), but I could be wrong.
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -287,6 +293,51 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
+ private void downloadFromZK(ConfigSetService configSetService, String configName, URI dir) throws IOException {
Review comment:
Instead of "ZK" in the name here, let's say "Solr". Same for upload.
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -287,6 +293,51 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
+ private void downloadFromZK(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> files = configSetService.listFilesInConfig(configName);
+ for (String file : files) {
+ List<String> children = configSetService.listFilesInConfig(configName, file);
+ if (children.size() == 0) {
+ log.debug("Writing file {}", file);
+ byte[] data = configSetService.getFileFromConfig(configName, file);
+ try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
+ os.write(data);
+ }
+ } else {
+ URI uri = repository.resolve(dir, file);
+ if (!repository.exists(uri)) {
+ repository.createDirectory(uri);
+ }
+ downloadFromZK(configSetService, configName, repository.resolve(dir, file));
+ }
+ }
+ }
+
+ private void uploadToZk(ConfigSetService configSetService, URI sourceDir, String configName) throws IOException {
+ for (String file : repository.listAll(sourceDir)) {
+ URI path = repository.resolve(sourceDir, file);
+ BackupRepository.PathType t = repository.getPathType(path);
+ switch (t) {
+ case FILE: {
+ try (IndexInput is = repository.openInput(sourceDir, file, IOContext.DEFAULT)) {
+ byte[] arr = new byte[(int) is.length()]; // probably ok since the config file should be small.
Review comment:
Yuck but it seems the code was this way already.
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -287,6 +293,51 @@ public void downloadCollectionProperties(String collectionName) throws IOExcepti
}
}
+ private void downloadFromZK(ConfigSetService configSetService, String configName, URI dir) throws IOException {
+ List<String> files = configSetService.listFilesInConfig(configName);
+ for (String file : files) {
+ List<String> children = configSetService.listFilesInConfig(configName, file);
+ if (children.size() == 0) {
+ log.debug("Writing file {}", file);
+ byte[] data = configSetService.getFileFromConfig(configName, file);
+ try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
+ os.write(data);
+ }
+ } else {
+ URI uri = repository.resolve(dir, file);
+ if (!repository.exists(uri)) {
+ repository.createDirectory(uri);
+ }
+ downloadFromZK(configSetService, configName, repository.resolve(dir, file));
+ }
+ }
+ }
+
+ private void uploadToZk(ConfigSetService configSetService, URI sourceDir, String configName) throws IOException {
+ for (String file : repository.listAll(sourceDir)) {
+ URI path = repository.resolve(sourceDir, file);
+ BackupRepository.PathType t = repository.getPathType(path);
+ switch (t) {
+ case FILE: {
+ try (IndexInput is = repository.openInput(sourceDir, file, IOContext.DEFAULT)) {
+ byte[] arr = new byte[(int) is.length()]; // probably ok since the config file should be small.
Review comment:
BTW @bruno-roustant , this is another place where that InputStream wrapper around an IndexInput could be useful in order to use common APIs that work with InputStreams. I'm reminded of this from your BlobDirectory work.
--
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
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603904570
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -242,8 +243,7 @@ public static void main(String[] args) throws InterruptedException,
}
String confDir = line.getOptionValue(CONFDIR);
String confName = line.getOptionValue(CONFNAME);
- ZkConfigManager configManager = new ZkConfigManager(zkClient);
- configManager.downloadConfigDir(confName, Paths.get(confDir));
+ ZkMaintenanceUtils.downloadFromZK(zkClient, ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, Paths.get(confDir));
Review comment:
But I need to override uploadConfig as the last param is excludePattern
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] NazerkeBS commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r603906237
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
##########
@@ -231,9 +233,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);
+ ZkMaintenanceUtils.uploadToZK(zkClient, Paths.get(confDir), ZkMaintenanceUtils.CONFIGS_ZKNODE + "/" + confName, excludePattern);
Review comment:
In this case, I have to overload uploadConfig method to add excludePattern param in ConfigSetService
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r602653831
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -276,20 +270,11 @@ public void downloadConfig(String configName, Path dir) throws IOException {
throw new IllegalArgumentException("\"" + zkPath + "\" not recognized as a configset path");
Review comment:
This `if` condition is impossible since we built zkPath
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -399,11 +384,10 @@ private void copyConfigDirFromZk(String fromZkPath, String toZkPath, Set<String>
}
}
- private void copyData(Set<String> copiedToZkPaths, String fromZkFilePath, String toZkFilePath) throws KeeperException, InterruptedException {
+ private void copyData(String fromZkFilePath, String toZkFilePath) throws KeeperException, InterruptedException {
log.info("Copying zk node {} to {}", fromZkFilePath, toZkFilePath);
Review comment:
I'm skeptical an INFO level log is appropriate here. I think DEBUG.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -276,20 +270,11 @@ public void downloadConfig(String configName, Path dir) throws IOException {
throw new IllegalArgumentException("\"" + zkPath + "\" not recognized as a configset path");
}
try {
- return zkClient.getAllConfigsetFiles(zkPath);
- } catch (KeeperException | InterruptedException e) {
- throw new IOException("Error getting all configset files", SolrZkClient.checkInterrupted(e));
- }
- }
-
- @Override
- public String getConfigPath(String configName) throws IOException {
- try {
- if (zkClient.exists(CONFIGS_ZKNODE + "/" + configName, true)) {
- return CONFIGS_ZKNODE + "/" + configName;
- } else {
- throw new IOException("config does not exist");
+ List<String> filePaths = new LinkedList<>();
Review comment:
Why LinkedList?
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -276,20 +270,11 @@ public void downloadConfig(String configName, Path dir) throws IOException {
throw new IllegalArgumentException("\"" + zkPath + "\" not recognized as a configset path");
}
try {
- return zkClient.getAllConfigsetFiles(zkPath);
- } catch (KeeperException | InterruptedException e) {
- throw new IOException("Error getting all configset files", SolrZkClient.checkInterrupted(e));
- }
- }
-
- @Override
- public String getConfigPath(String configName) throws IOException {
- try {
- if (zkClient.exists(CONFIGS_ZKNODE + "/" + configName, true)) {
- return CONFIGS_ZKNODE + "/" + configName;
- } else {
- throw new IOException("config does not exist");
+ List<String> filePaths = new LinkedList<>();
+ for (String filePath : zkClient.getAllConfigsetFiles(zkPath)) {
+ filePaths.add(filePath.replaceAll(zkPath + "/", ""));
Review comment:
I think this will replace any substring of the filePath? I think we want to specifically chop off the beginning; no? I'd prefer you assert that each filePath starts with what you think it does, then truncate it off.
Also, I don't see why SolrZkClient needs a getAllConfigsetFiles method. From what I see, we could call `org.apache.solr.common.cloud.ZkMaintenanceUtils#traverseZkTree` which does *not* prefix it's results with the zkPath argument you give it.
But morover
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -325,54 +312,44 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
/**
* Delete files in config
*
+ *
+ * @param configName the name of the config
* @param filesToDelete a list of file paths to delete
- * @throws IOException if an I/O error occurs
*/
- public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
+ public abstract void deleteFilesFromConfig(String configName, List<String> filesToDelete) throws IOException;
/**
* Set the config metadata
* If config does not exist, it will be created and set metadata on it
- * Else metadata will be updated
+ * Else metadata will be replaced with the provided metadata
* @param configName the config name
* @param data the metadata to be set on config
- * @throws IOException if an I/O error occurs
*/
public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
* Get the config metadata
*
* @param configName the config name
- * @return the config metadata
+ * @return the config metadata (mutable) or null
* @throws IOException if an I/O error occurs or config does not exist
*/
public abstract Map<String, Object> getConfigMetadata(String configName) throws IOException;
/**
* List the names of configs
*
- * @return list the names of configs
- * @throws IOException if an I/O error occurs
+ * @return list the names of configs or empty list
*/
public abstract List<String> listConfigs() throws IOException;
/**
- * Get all files in config
+ * Get the names of the files in config
* @param configName the config name
- * @return list of file paths under configName excluding itself
- * @throws IOException if an I/O error occurs
+ * @return list of file name paths in the config e.g. foo/managed-schema, foo/foo2/solrconfig.xml
Review comment:
BTW I suggest putting examples in the description portion of the javadoc, and leave the \@return and other elements more brief and to the point. Mentioning nullability and mutability is useful.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -325,54 +312,44 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
/**
* Delete files in config
*
+ *
+ * @param configName the name of the config
* @param filesToDelete a list of file paths to delete
- * @throws IOException if an I/O error occurs
*/
- public abstract void deleteFilesFromConfig(List<String> filesToDelete) throws IOException;
+ public abstract void deleteFilesFromConfig(String configName, List<String> filesToDelete) throws IOException;
/**
* Set the config metadata
* If config does not exist, it will be created and set metadata on it
- * Else metadata will be updated
+ * Else metadata will be replaced with the provided metadata
* @param configName the config name
* @param data the metadata to be set on config
- * @throws IOException if an I/O error occurs
*/
public abstract void setConfigMetadata(String configName, Map<String, Object> data) throws IOException;
/**
* Get the config metadata
*
* @param configName the config name
- * @return the config metadata
+ * @return the config metadata (mutable) or null
* @throws IOException if an I/O error occurs or config does not exist
*/
public abstract Map<String, Object> getConfigMetadata(String configName) throws IOException;
/**
* List the names of configs
*
- * @return list the names of configs
- * @throws IOException if an I/O error occurs
+ * @return list the names of configs or empty list
*/
public abstract List<String> listConfigs() throws IOException;
/**
- * Get all files in config
+ * Get the names of the files in config
* @param configName the config name
- * @return list of file paths under configName excluding itself
- * @throws IOException if an I/O error occurs
+ * @return list of file name paths in the config e.g. foo/managed-schema, foo/foo2/solrconfig.xml
Review comment:
In practice, managed-schema & solrconfig.xml will be at the root. A realistic example of a configSet is here: https://github.com/apache/solr/tree/9491cc116e9cdd9c46a073f9c4b54df6a093193b/solr/server/solr/configsets/sample_techproducts_configs/conf notice that "lang" is a realistic intermediate directory name.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #23: SOLR-15258: ConfigSetService add CRUD operations, subsume ZkConfigManager
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r598745515
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -253,7 +253,7 @@ public void downloadConfigDir(String configName) throws IOException {
repository.createDirectory(getZkStateDir(CONFIG_STATE_DIR));
repository.createDirectory(dest);
- downloadFromZK(zkStateReader.getZkClient(), ZkConfigManager.CONFIGS_ZKNODE + "/" + configName, dest);
+ downloadFromZK(zkStateReader.getZkClient(), ZkConfigSetService.CONFIGS_ZKNODE + "/" + configName, dest);
Review comment:
BackupManager should be using configSetService completely to manage configSets, not ZK
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java
##########
@@ -301,7 +302,7 @@ static void deleteBackup(BackupRepository repository, URI backupPath, int maxNum
}
static void validateConfigOrThrowSolrException(SolrCloudManager cloudManager, String configName) throws IOException, KeeperException, InterruptedException {
- boolean isValid = cloudManager.getDistribStateManager().hasData(ZkConfigManager.CONFIGS_ZKNODE + "/" + configName);
+ boolean isValid = cloudManager.getDistribStateManager().hasData(ZkConfigSetService.CONFIGS_ZKNODE + "/" + configName);
Review comment:
bad dependency on where configSets are located; use configSetService. Maybe SolrCloudManager should have CSS?
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerCollectionConfigSetProcessor.java
##########
@@ -88,9 +88,9 @@ private static OverseerMessageHandlerSelector getOverseerMessageHandlerSelector(
Overseer overseer,
OverseerNodePrioritizer overseerNodePrioritizer) {
final OverseerCollectionMessageHandler collMessageHandler = new OverseerCollectionMessageHandler(
- zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer);
+ zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer, overseer.getCoreContainer());
final OverseerConfigSetMessageHandler configMessageHandler = new OverseerConfigSetMessageHandler(
- zkStateReader);
+ zkStateReader, overseer.getCoreContainer());
Review comment:
Can't we just pass ConfigSetService?
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerConfigSetMessageHandler.java
##########
@@ -286,7 +288,7 @@ private void mergeOldProperties(Map<String, Object> newProps, @SuppressWarnings(
}
private String getPropertyPath(String configName, String propertyPath) {
- return ZkConfigManager.CONFIGS_ZKNODE + "/" + configName + "/" + propertyPath;
+ return ZkConfigSetService.CONFIGS_ZKNODE + "/" + configName + "/" + propertyPath;
Review comment:
Suspicious. getConfigSetProperties is assuming ZK. Review this whole class to ensure ZK references are reconsidered.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -0,0 +1,319 @@
+/*
+ * 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.File;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+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.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.CoreContainer;
+import org.apache.solr.core.CoreDescriptor;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.servlet.SolrDispatchFilter;
+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 final ZkController zkController;
+ private final SolrZkClient zkClient;
+ /** 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(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.zkController = cc.getZkController();
+ this.zkClient = cc.getZkController().getZkClient();
+ try {
+ bootstrapDefaultConfigSet();
+ } catch (IOException e) {
+ log.error("Error in bootstrapping default config");
+ }
+ }
+
+ /** This is for ZkCLI and some tests */
+ public ZkConfigSetService(SolrZkClient zkClient) {
+ super(null, false);
+ this.zkController = null;
+ this.zkClient = zkClient;
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ final String colName = cd.getCollectionName();
+
+ // For back compat with cores that can create collections without the collections API
+ try {
+ if (!zkClient.exists(ZkStateReader.COLLECTIONS_ZKNODE + "/" + colName, true)) {
+ // TODO remove this functionality or maybe move to a CLI mechanism
+ log.warn("Auto-creating collection (in ZK) from core descriptor (on disk). This feature may go away!");
+ CreateCollectionCmd.createCollectionZkNode(zkController.getSolrCloudManager().getDistribStateManager(), colName, cd.getCloudDescriptor().getParams());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Interrupted auto-creating collection", e);
+ } catch (KeeperException e) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Failure auto-creating collection", e);
+ }
+
+ // The configSet is read from ZK and populated. Ignore CD's pre-existing configSet; only populated in standalone
+ final String configSetName;
+ try {
+ configSetName = zkController.getZkStateReader().readConfigName(colName);
+ cd.setConfigSet(configSetName);
+ } catch (KeeperException ex) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Trouble resolving configSet for collection " + colName + ": " + ex.getMessage());
+ }
+
+ return new ZkSolrResourceLoader(cd.getInstanceDir(), configSetName, parentLoader.getClassLoader(), zkController);
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes"})
+ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loader) {
+ try {
+ return ConfigSetProperties.readFromResourceLoader(loader, ".");
+ } catch (Exception ex) {
+ log.debug("No configSet flags", ex);
+ return null;
+ }
+ }
+
+ @Override
+ protected Long getCurrentSchemaModificationVersion(String configSet, SolrConfig solrConfig, String schemaFile) {
+ String zkPath = CONFIGS_ZKNODE + "/" + configSet + "/" + schemaFile;
+ Stat stat;
+ try {
+ stat = zkClient.exists(zkPath, null, true);
+ } catch (KeeperException e) {
+ log.warn("Unexpected exception when getting modification time of {}", zkPath, e);
+ return null; // debatable; we'll see an error soon if there's a real problem
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ if (stat == null) { // not found
+ return null;
+ }
+ return (long) stat.getVersion();
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return "configset " + cd.getConfigSet();
+ }
+
+ @Override
+ public boolean checkConfigExists(String configName) throws IOException {
+ try {
+ return zkClient.exists(CONFIGS_ZKNODE + "/" + configName, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void deleteConfigDir(String configName) throws IOException {
+ try {
+ zkClient.clean(CONFIGS_ZKNODE + "/" + configName);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig) throws IOException {
+ copyConfigDir(fromConfig, toConfig, null);
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException {
+ String fromConfigPath = CONFIGS_ZKNODE + "/" + fromConfig;
+ String toConfigPath = CONFIGS_ZKNODE + "/" + toConfig;
+ try {
+ copyData(copiedToZkPaths, fromConfigPath, toConfigPath);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error config " + fromConfig + " to " + toConfig,
+ SolrZkClient.checkInterrupted(e));
+ }
+ copyConfigDirFromZk(fromConfigPath, toConfigPath, copiedToZkPaths);
+ }
+
+ @Override
+ public void uploadConfigDir(Path dir, String configName) throws IOException {
+ zkClient.uploadToZK(dir, CONFIGS_ZKNODE + "/" + configName, ZkConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN);
+ }
+
+ @Override
+ public void downloadConfigDir(String configName, Path dir) throws IOException {
+ zkClient.downloadFromZK(CONFIGS_ZKNODE + "/" + configName, dir);
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ try {
+ return zkClient.getChildren(CONFIGS_ZKNODE, null, true);
+ } catch (KeeperException.NoNodeException e) {
+ return Collections.emptyList();
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error listing configs", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ private void bootstrapDefaultConfigSet() throws IOException {
Review comment:
Doesn't seem specific to ZK.
##########
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:
Yes; do that and to the other methods with "Dir"
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -167,7 +167,7 @@ private void handleConfigUploadRequest(SolrQueryRequest req, SolrQueryResponse r
}
SolrZkClient zkClient = coreContainer.getZkController().getZkClient();
- String configPathInZk = ZkConfigManager.CONFIGS_ZKNODE + "/" + configSetName;
+ String configPathInZk = ZkConfigSetService.CONFIGS_ZKNODE + "/" + configSetName;
Review comment:
Should use ConfigSetService abstraction
##########
File path: solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.core;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.solr.common.SolrException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The Solr standalone version of File System ConfigSetService.
+ * <p>
+ * Loads a ConfigSet defined by the core's configSet property,
+ * looking for a directory named for the configSet property value underneath
+ * a base directory. If no configSet property is set, loads the ConfigSet
+ * instead from the core's instance directory.
+ */
+public class FileSystemConfigSetService extends ConfigSetService {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private final Path configSetBase;
+
+ public FileSystemConfigSetService(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ Path instanceDir = locateInstanceDir(cd);
+ SolrResourceLoader solrResourceLoader = new SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
+ return solrResourceLoader;
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return (cd.getConfigSet() == null ? "instancedir " : "configset ") + locateInstanceDir(cd);
+ }
+
+ @Override
+ public void deleteConfigDir(String configName) throws IOException {
+ throw new UnsupportedOperationException("Only supported with ZkConfigSetService");
Review comment:
Let's not mention other ConfigSetService impls. I dunno if UOE can have no arg at all; we don't need to say anything special.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -216,72 +237,76 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
*/
public abstract String configSetName(CoreDescriptor cd);
- public interface ConfigResource {
-
- ConfigNode get() throws Exception;
-
+ /**
+ * Check whether a config exists
+ *
+ * @param configName the config to check existance on
+ * @return whether the config exists or not
+ * @throws IOException if an I/O error occurs
+ */
+ public boolean checkConfigExists(String configName) throws IOException {
+ return listConfigs().contains(configName);
}
- public static ConfigNode getParsedSchema(InputStream is, SolrResourceLoader loader, String name) throws IOException, SAXException, ParserConfigurationException {
- XmlConfigFile schemaConf = null;
- InputSource inputSource = new InputSource(is);
- inputSource.setSystemId(SystemIdResolver.createSystemIdFromResourceName(name));
- schemaConf = new XmlConfigFile(loader, SCHEMA, inputSource, SLASH + SCHEMA + SLASH, null);
- return new DataConfigNode(new DOMConfigNode(schemaConf.getDocument().getDocumentElement()));
- }
+ /**
+ * Delete a config
+ *
+ * @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
+ *
+ * @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;
/**
- * The Solr standalone version of ConfigSetService.
+ * Copy a config
*
- * Loads a ConfigSet defined by the core's configSet property,
- * looking for a directory named for the configSet property value underneath
- * a base directory. If no configSet property is set, loads the ConfigSet
- * instead from the core's instance directory.
+ * @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 static class Standalone extends ConfigSetService {
+ public abstract void copyConfigDir(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException;
- private final Path configSetBase;
+ /**
+ * Upload files from a given path to a config
+ *
+ * @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;
- public Standalone(SolrResourceLoader loader, boolean shareSchema, Path configSetBase) {
- super(loader, shareSchema);
- this.configSetBase = configSetBase;
- }
+ /**
+ * Download a config from Zookeeper and write it to the filesystem
+ *
+ * @param configName the config to download
+ * @param dir the {@link Path} to write files under
+ * @throws IOException if an I/O error occurs or the config does not exist
+ */
+ public abstract void downloadConfigDir(String configName, Path dir) throws IOException;
- @Override
- public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
- Path instanceDir = locateInstanceDir(cd);
- SolrResourceLoader solrResourceLoader = new SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
- return solrResourceLoader;
- }
+ public abstract List<String> listConfigs() throws IOException;
- @Override
- public String configSetName(CoreDescriptor cd) {
- return (cd.getConfigSet() == null ? "instancedir " : "configset ") + locateInstanceDir(cd);
- }
+ public interface ConfigResource {
- protected Path locateInstanceDir(CoreDescriptor cd) {
- String configSet = cd.getConfigSet();
- if (configSet == null)
- return cd.getInstanceDir();
- Path configSetDirectory = configSetBase.resolve(configSet);
- if (!Files.isDirectory(configSetDirectory))
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
- "Could not load configuration from directory " + configSetDirectory);
- return configSetDirectory;
- }
+ ConfigNode get() throws Exception;
- @Override
- protected Long getCurrentSchemaModificationVersion(String configSet, SolrConfig solrConfig, String schemaFileName) {
- Path schemaFile = Paths.get(solrConfig.getResourceLoader().getConfigDir()).resolve(schemaFileName);
- try {
- return Files.getLastModifiedTime(schemaFile).toMillis();
- } catch (FileNotFoundException e) {
- return null; // acceptable
- } catch (IOException e) {
- log.warn("Unexpected exception when getting modification time of {}", schemaFile, e);
- return null; // debatable; we'll see an error soon if there's a real problem
- }
- }
+ }
+ public static ConfigNode getParsedSchema(InputStream is, SolrResourceLoader loader, String name) throws IOException, SAXException, ParserConfigurationException {
Review comment:
This method seems out of place here... maybe move with the other static methods in this class.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -626,7 +625,7 @@ private static void getConfName(DistribStateManager stateManager, String collect
}
try {
- configNames = stateManager.listData(ZkConfigManager.CONFIGS_ZKNODE);
+ configNames = stateManager.listData(ZkConfigSetService.CONFIGS_ZKNODE);
Review comment:
No; use ConfigSetService.
##########
File path: solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.core;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.solr.common.SolrException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The Solr standalone version of File System ConfigSetService.
+ * <p>
+ * Loads a ConfigSet defined by the core's configSet property,
+ * looking for a directory named for the configSet property value underneath
+ * a base directory. If no configSet property is set, loads the ConfigSet
+ * instead from the core's instance directory.
+ */
+public class FileSystemConfigSetService extends ConfigSetService {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private final Path configSetBase;
+
+ public FileSystemConfigSetService(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ Path instanceDir = locateInstanceDir(cd);
+ SolrResourceLoader solrResourceLoader = new SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
+ return solrResourceLoader;
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return (cd.getConfigSet() == null ? "instancedir " : "configset ") + locateInstanceDir(cd);
+ }
+
+ @Override
+ public void deleteConfigDir(String configName) throws IOException {
+ throw new UnsupportedOperationException("Only supported with ZkConfigSetService");
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig) throws IOException {
+ throw new UnsupportedOperationException("Only supported with ZkConfigSetService");
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException {
+ throw new UnsupportedOperationException("Only supported with ZkConfigSetService");
+ }
+
+ @Override
+ public void uploadConfigDir(Path dir, String configName) throws IOException {
+ throw new UnsupportedOperationException("Only supported with ZkConfigSetService");
+ }
+
+ @Override
+ public void downloadConfigDir(String configName, Path dir) throws IOException {
+ throw new UnsupportedOperationException("Only supported with ZkConfigSetService");
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ List<String> configs = Files.list(configSetBase)
Review comment:
return directly without declaring
##########
File path: solr/core/src/test/org/apache/solr/cloud/api/collections/SimpleCollectionCreateDeleteTest.java
##########
@@ -141,7 +141,7 @@ public void testDeleteDoesNotDeleteSharedAutocreatedConfigSet() throws Exception
// config for this collection is '.AUTOCREATED', and exists globally
assertTrue(configName.endsWith(".AUTOCREATED"));
- assertTrue(cloudClient.getZkStateReader().getConfigManager().listConfigs().contains(configName));
Review comment:
There are a number of calls like this in tests. The changed code is now kind of worse but I understand why it couldn't stay the same. Perhaps SolrZkClient should have a listConfigs() ?
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -0,0 +1,319 @@
+/*
+ * 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.File;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+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.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.CoreContainer;
+import org.apache.solr.core.CoreDescriptor;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.servlet.SolrDispatchFilter;
+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 final ZkController zkController;
+ private final SolrZkClient zkClient;
+ /** 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(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.zkController = cc.getZkController();
+ this.zkClient = cc.getZkController().getZkClient();
+ try {
+ bootstrapDefaultConfigSet();
+ } catch (IOException e) {
+ log.error("Error in bootstrapping default config");
+ }
+ }
+
+ /** This is for ZkCLI and some tests */
+ public ZkConfigSetService(SolrZkClient zkClient) {
+ super(null, false);
+ this.zkController = null;
+ this.zkClient = zkClient;
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ final String colName = cd.getCollectionName();
+
+ // For back compat with cores that can create collections without the collections API
+ try {
+ if (!zkClient.exists(ZkStateReader.COLLECTIONS_ZKNODE + "/" + colName, true)) {
+ // TODO remove this functionality or maybe move to a CLI mechanism
+ log.warn("Auto-creating collection (in ZK) from core descriptor (on disk). This feature may go away!");
+ CreateCollectionCmd.createCollectionZkNode(zkController.getSolrCloudManager().getDistribStateManager(), colName, cd.getCloudDescriptor().getParams());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Interrupted auto-creating collection", e);
+ } catch (KeeperException e) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Failure auto-creating collection", e);
+ }
+
+ // The configSet is read from ZK and populated. Ignore CD's pre-existing configSet; only populated in standalone
+ final String configSetName;
+ try {
+ configSetName = zkController.getZkStateReader().readConfigName(colName);
+ cd.setConfigSet(configSetName);
+ } catch (KeeperException ex) {
+ throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Trouble resolving configSet for collection " + colName + ": " + ex.getMessage());
+ }
+
+ return new ZkSolrResourceLoader(cd.getInstanceDir(), configSetName, parentLoader.getClassLoader(), zkController);
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes"})
+ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loader) {
+ try {
+ return ConfigSetProperties.readFromResourceLoader(loader, ".");
+ } catch (Exception ex) {
+ log.debug("No configSet flags", ex);
+ return null;
+ }
+ }
+
+ @Override
+ protected Long getCurrentSchemaModificationVersion(String configSet, SolrConfig solrConfig, String schemaFile) {
+ String zkPath = CONFIGS_ZKNODE + "/" + configSet + "/" + schemaFile;
+ Stat stat;
+ try {
+ stat = zkClient.exists(zkPath, null, true);
+ } catch (KeeperException e) {
+ log.warn("Unexpected exception when getting modification time of {}", zkPath, e);
+ return null; // debatable; we'll see an error soon if there's a real problem
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ if (stat == null) { // not found
+ return null;
+ }
+ return (long) stat.getVersion();
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return "configset " + cd.getConfigSet();
+ }
+
+ @Override
+ public boolean checkConfigExists(String configName) throws IOException {
+ try {
+ return zkClient.exists(CONFIGS_ZKNODE + "/" + configName, true);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void deleteConfigDir(String configName) throws IOException {
+ try {
+ zkClient.clean(CONFIGS_ZKNODE + "/" + configName);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error checking whether config exists",
+ SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig) throws IOException {
+ copyConfigDir(fromConfig, toConfig, null);
+ }
+
+ @Override
+ public void copyConfigDir(String fromConfig, String toConfig, Set<String> copiedToZkPaths) throws IOException {
+ String fromConfigPath = CONFIGS_ZKNODE + "/" + fromConfig;
+ String toConfigPath = CONFIGS_ZKNODE + "/" + toConfig;
+ try {
+ copyData(copiedToZkPaths, fromConfigPath, toConfigPath);
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error config " + fromConfig + " to " + toConfig,
+ SolrZkClient.checkInterrupted(e));
+ }
+ copyConfigDirFromZk(fromConfigPath, toConfigPath, copiedToZkPaths);
+ }
+
+ @Override
+ public void uploadConfigDir(Path dir, String configName) throws IOException {
+ zkClient.uploadToZK(dir, CONFIGS_ZKNODE + "/" + configName, ZkConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN);
+ }
+
+ @Override
+ public void downloadConfigDir(String configName, Path dir) throws IOException {
+ zkClient.downloadFromZK(CONFIGS_ZKNODE + "/" + configName, dir);
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ try {
+ return zkClient.getChildren(CONFIGS_ZKNODE, null, true);
+ } catch (KeeperException.NoNodeException e) {
+ return Collections.emptyList();
+ } catch (KeeperException | InterruptedException e) {
+ throw new IOException("Error listing configs", SolrZkClient.checkInterrupted(e));
+ }
+ }
+
+ private void bootstrapDefaultConfigSet() throws IOException {
+ if (this.checkConfigExists("_default") == false) {
+ String configDirPath = getDefaultConfigDirPath();
+ if (configDirPath == null) {
+ log.warn(
+ "The _default configset could not be uploaded. Please provide 'solr.default.confdir' parameter that points to a configset {} {}",
+ "intended to be the default. Current 'solr.default.confdir' value:",
+ System.getProperty(SolrDispatchFilter.SOLR_DEFAULT_CONFDIR_ATTRIBUTE));
+ } else {
+ this.uploadConfigDir(Paths.get(configDirPath), ConfigSetsHandler.DEFAULT_CONFIGSET_NAME);
+ }
+ }
+ }
+
+ /**
+ * Gets the absolute filesystem path of the _default configset to bootstrap from. First tries the
+ * sysprop "solr.default.confdir". If not found, tries to find the _default dir relative to the
+ * sysprop "solr.install.dir". Returns null if not found anywhere.
+ *
+ * @lucene.internal
+ * @see SolrDispatchFilter#SOLR_DEFAULT_CONFDIR_ATTRIBUTE
+ */
+ public static String getDefaultConfigDirPath() {
Review comment:
Looks like it belongs on the FileSystem one? This isn't ZK related.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java
##########
@@ -67,6 +68,7 @@
HttpShardHandlerFactory shardHandlerFactory;
String adminPath;
ZkStateReader zkStateReader;
+ CoreContainer cc;
Review comment:
Not used?
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerCollectionConfigSetProcessor.java
##########
@@ -88,9 +88,9 @@ private static OverseerMessageHandlerSelector getOverseerMessageHandlerSelector(
Overseer overseer,
OverseerNodePrioritizer overseerNodePrioritizer) {
final OverseerCollectionMessageHandler collMessageHandler = new OverseerCollectionMessageHandler(
- zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer);
+ zkStateReader, myId, shardHandlerFactory, adminPath, stats, overseer, overseerNodePrioritizer, overseer.getCoreContainer());
Review comment:
Why does it need this?
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -476,7 +477,7 @@ String getConfigName(String coll, ZkNodeProps message) throws KeeperException, I
// if there is only one conf, use that
List<String> configNames = null;
try {
- configNames = ccc.getZkStateReader().getZkClient().getChildren(ZkConfigManager.CONFIGS_ZKNODE, null, true);
Review comment:
No; should be using configSetService to list configs
##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerConfigSetMessageHandler.java
##########
@@ -70,6 +70,7 @@
public static final String PROPERTY_PREFIX = "configSetProp";
private ZkStateReader zkStateReader;
+ private CoreContainer cc;
Review comment:
ConfigSetService instead
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPIZkFailure.java
##########
@@ -111,14 +110,14 @@ public void testCreateZkFailure() throws Exception {
SolrZkClient zkClient = new SolrZkClient(solrCluster.getZkServer().getZkAddress(),
AbstractZkTestCase.TIMEOUT, AbstractZkTestCase.TIMEOUT, null);
try {
- ZkConfigManager configManager = new ZkConfigManager(zkClient);
- assertFalse(configManager.configExists(CONFIGSET_NAME));
+
+ assertFalse(solrCluster.getOpenOverseer().getCoreContainer().getConfigSetService().checkConfigExists(CONFIGSET_NAME));
Review comment:
long and used twice. Declare `solrCluster.getOpenOverseer().getCoreContainer().getConfigSetService()`
##########
File path: solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java
##########
@@ -230,7 +230,7 @@ private void testNoConfigset() throws Exception {
waitForRecoveriesToFinish(collection, false);
// Now try deleting the configset and doing a clusterstatus.
- String parent = ZkConfigManager.CONFIGS_ZKNODE + "/" + configSet;
+ String parent = ZkConfigSetService.CONFIGS_ZKNODE + "/" + configSet;
Review comment:
Could we create a ZkConfigSetService to call convenient APIs for this?
##########
File path: solr/solr-ref-guide/src/format-of-solr-xml.adoc
##########
@@ -68,7 +68,7 @@ This attribute does not need to be set.
+
If used, this attribute should be set to the FQN (Fully qualified name) of a class that inherits from ConfigSetService , and you must provide a constructor with one param which the type is `org.apache.solr.core.CoreContainer` . For example, `<str name="configSetService">com.myorg.CustomConfigSetService</str>`.
+
-If this attribute isn't set, Solr uses the default configSetService , with zookeeper aware of `org.apache.solr.cloud.CloudConfigSetService`, without zookeeper aware of `org.apache.solr.core.ConfigSetService.Standalone`.
+If this attribute isn't set, Solr uses the default configSetService , with zookeeper aware of `org.apache.solr.cloud.ZkConfigSetService`, without zookeeper aware of `org.apache.solr.core.ConfigSetService.FileSystemConfigSetService`.
Review comment:
The FS one is no longer an inner class.
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java
##########
@@ -293,8 +292,7 @@ private NamedList getConfigSetPropertiesFromZk(
private void verifyProperties(String configSetName, Map<String, String> oldProps,
Map<String, String> newProps, SolrZkClient zkClient) throws Exception {
@SuppressWarnings({"rawtypes"})
- NamedList properties = getConfigSetPropertiesFromZk(zkClient,
- ZkConfigManager.CONFIGS_ZKNODE + "/" + configSetName + "/" + DEFAULT_FILENAME);
+ NamedList properties = getConfigSetPropertiesFromZk(zkClient,ZkConfigSetService.CONFIGS_ZKNODE + "/" + configSetName + "/" + DEFAULT_FILENAME);
Review comment:
`getConfigSetPropertiesFromZk` circumvents the ConfigSetService; could it use 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org