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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
