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