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