dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r604034499
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -344,8 +344,9 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd,
SolrResourceLoader loa
public abstract List<String> listConfigs() throws IOException;
/**
- * Get the names of the files in config including dir (mutable, non-null)
- * e.g. solrconfig.xml, lang/stopwords_en.txt, lang/stoptags_en.txt
+ * Get the names of the files in config including dirs (mutable, non-null)
+ * Sorted lexicographically
+ * e.g. solrconfig.xml, lang/, lang/stoptags_en.txt, lang/stopwords_en.txt
Review comment:
oh more more little thing. I've never seen a file in Solr called
"stoptags_en.txt" and just recently noticed there is one or two
"stoptags_ja.txt" so that would better. But it's extra; we only need _one_
file in the example in the subdir, so stopwords_en.txt is common & sufficient
without "stoptags" (whatever that is).
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -277,17 +277,22 @@ public void downloadConfig(String configName, Path dir)
throws IOException {
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);
- // if it is a directory, concatenate '/' to the filePath
- if (zkClient.getChildren(filePath, null, true).size() != 0) {
- filePath = filePath + "/";
- }
+ String prevPath = null;
+ for (int i = 0; i < filePaths.size(); i++) {
+ String currPath = filePaths.get(i);
+
// stripping /configs/configName/
- assert(filePath.startsWith(zkPath + "/"));
- filePath = filePath.substring(zkPath.length()+1);
- filePaths.set(i, filePath);
+ assert (currPath.startsWith(zkPath + "/"));
Review comment:
The wrapping parenthesis is weird; please remove them.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -277,17 +277,22 @@ public void downloadConfig(String configName, Path dir)
throws IOException {
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);
- // if it is a directory, concatenate '/' to the filePath
- if (zkClient.getChildren(filePath, null, true).size() != 0) {
- filePath = filePath + "/";
- }
+ String prevPath = null;
+ for (int i = 0; i < filePaths.size(); i++) {
+ String currPath = filePaths.get(i);
+
// stripping /configs/configName/
- assert(filePath.startsWith(zkPath + "/"));
- filePath = filePath.substring(zkPath.length()+1);
- filePaths.set(i, filePath);
+ assert (currPath.startsWith(zkPath + "/"));
+ currPath = currPath.substring(zkPath.length() + 1);
+
+ // if currentPath is a directory, concatenate '/'
+ if (prevPath != null && prevPath.contains(currPath)) {
Review comment:
No, not "contains"; lets be more specific.
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java
##########
@@ -277,17 +277,22 @@ public void downloadConfig(String configName, Path dir)
throws IOException {
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);
- // if it is a directory, concatenate '/' to the filePath
- if (zkClient.getChildren(filePath, null, true).size() != 0) {
- filePath = filePath + "/";
- }
+ String prevPath = null;
Review comment:
If you use the empty string, it will simplify the below code slightly.
##########
File path: solr/core/src/java/org/apache/solr/core/backup/BackupManager.java
##########
@@ -323,15 +323,13 @@ private void uploadConfigToConfigRepo(ConfigSetService
configSetService, URI sou
is.readBytes(arr, 0, (int) is.length());
if(filePath.startsWith("/")) {
Review comment:
I think you misunderstand what I was trying to say. Can you see how
that leading "/" got there in the first place? (hint; it's earlier in this
very method). So my advise is to not put it there in the first place (instead
of always removing later).
--
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]
