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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
