rzo1 commented on code in PR #2118:
URL: https://github.com/apache/stormcrawler/pull/2118#discussion_r3944168276
##########
external/tika/src/main/resources/tika-config.json:
##########
@@ -0,0 +1,9 @@
+{
Review Comment:
This drops half of the config it replaces.
The deleted `tika-config.xml` carried two things this does not:
```xml
<service-loader initializableProblemHandler="ignore" loadErrorHandler="warn"
/>
<parser class="org.apache.tika.parser.CompositeParser">
<parser-exclude class="org.apache.tika.parser.ocr.TesseractOCRParser"/>
```
Losing the service-loader handlers puts Tika warnings back on stderr in
every worker, which the XML deliberately suppressed. Please carry both across,
or explain why Tika 4 makes them unnecessary.
##########
external/tika/src/main/java/org/apache/stormcrawler/tika/ParserBolt.java:
##########
@@ -343,6 +343,22 @@ private Tika instantiateTika(Map<String, Object> conf) {
return tika;
}
+ /**
+ * Returns the configuration file as a path, copying it to a temporary
file first if it is
+ * bundled inside a jar, as TikaLoader can only read configs from the
filesystem.
+ */
+ private Path urlToPath(URL configUrl) throws IOException,
URISyntaxException {
Review Comment:
This writes a temp file every time `instantiateTika` runs and relies on
`deleteOnExit`, which never fires for a killed worker. One file per bolt
instance per restart, accumulating in `/tmp` with default permissions.
```suggestion
private Path urlToPath(URL configUrl) throws IOException,
URISyntaxException {
if ("file".equals(configUrl.getProtocol())) {
return Paths.get(configUrl.toURI());
}
Path tmp = Files.createTempFile("tika-config", ".json");
try (InputStream is = configUrl.openStream()) {
Files.copy(is, tmp, StandardCopyOption.REPLACE_EXISTING);
}
return tmp;
}
```
and delete it in a `finally` in `instantiateTika` once `TikaLoader.load` has
returned, rather than deferring to JVM exit.
##########
external/tika/src/main/java/org/apache/stormcrawler/tika/ParserBolt.java:
##########
@@ -317,16 +316,17 @@ public void execute(Tuple tuple) {
private Tika instantiateTika(Map<String, Object> conf) {
Tika tika = null;
String tikaConfigFile =
- ConfUtils.getString(conf, "parser.tika.config.file",
"tika-config.xml");
+ ConfUtils.getString(conf, "parser.tika.config.file",
"tika-config.json");
Review Comment:
Pre-existing bug this PR carries forward: the code reads
`parser.tika.config.file`, but all three archetype `crawler-conf.yaml` files
write `parse.tika.config.file` (and this PR updates all three). The archetype
setting has never had any effect.
Since this PR touches all four places, please fix the key here too, or
accept both spellings for one release.
--
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]
