thomasmueller commented on code in PR #1716:
URL: https://github.com/apache/jackrabbit-oak/pull/1716#discussion_r1760717094
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/ConfigHelper.java:
##########
@@ -48,4 +52,11 @@ public static boolean getSystemPropertyAsBoolean(String
name, boolean defaultVal
LOG.info("Config {}={}", name, value);
return value;
}
+
+ public static List<String> getSystemPropertyAsStringList(String name,
String defaultValue, String separator) {
+ String result = System.getProperty(name, defaultValue);
+ List<String> parts = result.isBlank() ? List.of() :
Arrays.stream(result.split(separator)).map(String::trim).collect(Collectors.toList());
Review Comment:
What about to methods: `getSystemProperty` and `splitIgnoreSpace`? I see the
code is quite lenient in what it accepts (isBlank vs isEmpty, trim). For
user-facing configuration, I think it's the right thing to do; we ran into
problems because people added spaces eg. "type" = "lucene " (trailing space).
Having a separate method `splitIgnoreSpace` would be quite useful I think...
it would also simplify adding a test case just for this part.
(I think it is not necessary in this case because this is not end-user
facing but only for internal use. For internal use, I think non-lenient
behavior is better because it results in shorter, easier to read code. But it's
not that bad here, so I think it's fine.)
But more importantly, what if spaces have meaning? The method name
`getSystemPropertyAsStringList` doesn't convey what it does, and there is no
Javadoc.
What about renaming "separator" -> "separatorRegex", or then quote the
separator (using `Pattern.quote` I think)?
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMongoDownloadTask.java:
##########
@@ -786,6 +786,10 @@ void download(FindIterable<NodeDocument> mongoIterable)
throws InterruptedExcept
try {
while (cursor.hasNext()) {
NodeDocument next = cursor.next();
+ // If the id is not set, then the document was
filtered by NodeDocumentFilter and should be ignored
+ if (next.getId() == null) {
+ continue;
+ }
String id = next.getId();
Review Comment:
I would
```suggestion
String id = next.getId();
if (id == null) {
continue;
}
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
