tballison commented on PR #2118:
URL: https://github.com/apache/stormcrawler/pull/2118#issuecomment-5532217267
From my :robot:
```
Findings
1. parse-context in the user's JSON is silently ignored — ParserBolt:223.
Every parse builds new ParseContext() and sets only Parser.class and
HtmlMapper.class; the loader's parse-context (where Tika 4 puts output limits,
timeouts, exception reporting — the only new capabilities the JSON format
brings) never reaches it. A user who adds "parse-context": {"output-limits":
{"writeLimit": 100000}} sees no effect and no warning. Fix: hold the
loader (or tikaLoader.loadParseContext()) next to tika and seed each
per-parse context from it before setting Parser.class. Test: a config with a
small writeLimit, assert the extracted text is truncated. edge-case, in
scope.
2. A broken config degrades silently to Tika defaults — with OCR on —
instantiateTika, catch (Exception) → LOG.error → new Tika(). Pre-existing
pattern, but JSON changes its odds: Tika 4's mapper rejects unknown keys, so
one typo throws at load, and the topology quietly runs the full default
parser set, TesseractOCRParser included wherever tesseract is installed.
ParserBoltTest can't see this: it asserts tuple count and status, and passes
identically with tika-config.json deleted (verified by reading the test —
nothing checks what was loaded). Maintainer decision: fail fast in prepare()
(my recommendation for a crawler, where a silent config downgrade is
worse than a failed deploy) or keep the fallback; either way, a test that
asserts the loaded parser excludes TesseractOCRParser, so the config path is
proven rather than assumed. edge-case, in scope.
3. urlToPath keeps the temp copy for the worker's lifetime — deleteOnExit,
one file per bolt prepare(), never freed on a long-running worker.
TikaJsonConfig.load(Path) reads the file eagerly inside TikaLoader.load, so
delete it in a finally immediately after load returns. The workaround
itself is legitimate: at 4.0.0 TikaLoader's constructor is private and there's
no InputStream/TikaJsonConfig factory, even though
TikaJsonConfig.load(InputStream) exists — that's a Tika gap (punt list).
edge-case, in scope.
Hygiene (one line each):
- README links https://tika.apache.org/4.0.0/config.html → 404. The 4.0.0
index links https://tika.apache.org/docs/4.0.x/configuration/index.html (200).
- The three archetype crawler-conf.yamls set parse.tika.config.file; code
and docs read parser.tika.config.file, so the yaml value is ignored.
Pre-existing and harmless only because it equals the default — the PR edits
exactly those lines, so fix the key while there.
- Description: the dropped service-loader handlers aren't "the Tika 4
defaults" — at 4.0.0 ServiceLoader has no
LoadErrorHandler/InitializableProblemHandler at all, and the JSON
service-loader section deserializes into an
empty ServiceLoaderConfig. Dropping it is right; the reason is that it's
inert. Cosmetic.
- configuration.adoc says "Path to the Tika configuration file"; it's a
classpath resource name (getResource) — the README has it right.
```
One area where the bot is wrong is that configurations around timeouts don't
affect you because you aren't using tika pipes (we need to improve our
documentation around that). If you want process isolation, which is where we're
trying to move everyone, consider the `PipesForkParser`. It may not be a good
fit for StormCrawler. Happy to chat, though.
--
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]
