Copilot commented on code in PR #3082:
URL: https://github.com/apache/tika/pull/3082#discussion_r3874551565
##########
tika-pipes/tika-pipes-plugins/tika-pipes-file-system/src/test/java/org/apache/tika/pipes/fs/ConfigExamplesTest.java:
##########
@@ -42,4 +54,31 @@ public void testFileSystemEmitterConfig() throws Exception {
public void testFileSystemPipelineConfig() throws Exception {
loadAndValidate("file-system-pipeline.json");
}
+
+ @Test
+ public void testFileSystemJsonlReporterConfig() throws Exception {
+ loadAndValidate("file-system-jsonl-reporter.json");
+
+ JsonNode inner =
innerComponent(readExample("file-system-jsonl-reporter.json"),
+ "pipes-reporters", null, "file-system-jsonl-reporter");
+ FileSystemJsonlReporterConfig config =
FileSystemJsonlReporterConfig.load(inner.toString());
+ assertEquals("/var/log/tika/pipes-audit.jsonl",
config.path().toString());
Review Comment:
This assertion hard-codes the UNIX path string. On Windows,
`Path.toString()` will likely render with backslashes (e.g., `\var\log\...`),
making this test OS-dependent even though the example JSON is meant for
documentation only. Normalize separators (or assert on the raw JSON string) to
keep the test portable.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/async/AsyncProcessor.java:
##########
@@ -142,6 +145,9 @@ private AsyncProcessor(Path tikaConfigPath, PipesIterator
pipesIterator,
checkActive();
} catch (InterruptedException e) {
return WATCHER_FUTURE_CODE;
+ } catch (RuntimeException e) {
+ // already latched in failure; rethrowing would make
this future a second one
+ return WATCHER_FUTURE_CODE;
}
Review Comment:
The watcher thread catches *all* `RuntimeException`s from `checkActive()`
and exits quietly. That will also swallow unexpected runtime failures (e.g., an
IllegalArgumentException for an unknown future code), making the processor
continue without its watcher and hiding the root cause. Only suppress the
RuntimeException when the failure has already been latched; otherwise rethrow
so the error is surfaced.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/async/AsyncProcessor.java:
##########
@@ -343,8 +351,10 @@ public synchronized boolean checkActive() throws
InterruptedException {
throw new IllegalArgumentException("Don't recognize
this future code: " + i);
}
} catch (ExecutionException e) {
- LOG.error("execution exception", e);
- this.pipesReporter.error(e);
+ if (failure.compareAndSet(null, e)) {
+ LOG.error("execution exception", e);
+ this.pipesReporter.error(e);
+ }
Review Comment:
If `pipesReporter.error(e)` throws, it will escape this catch block and mask
the original `ExecutionException` from the worker/emitter. That makes
diagnosing the real failure harder and is inconsistent with the later logic
that avoids reporter exceptions masking the primary stop reason. Consider
catching reporter RuntimeExceptions here and suppressing them onto the original
`ExecutionException` before rethrowing.
--
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]
