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