rahil-c commented on code in PR #19678:
URL: https://github.com/apache/hudi/pull/19678#discussion_r3839245321
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/helpers/unstructured/TikaDocumentParser.java:
##########
@@ -40,9 +50,50 @@
public class TikaDocumentParser implements DocumentParser {
private static final long serialVersionUID = 1L;
+ private static final Logger LOG =
LoggerFactory.getLogger(TikaDocumentParser.class);
+
+ // one probe per JVM, not per task
+ private static final AtomicBoolean PROBED = new AtomicBoolean();
Review Comment:
It gated the parser-availability probe to one run per JVM rather than one
per task. Following the question, that gate was wrong: it recorded *that* a
probe had happened rather than *what it found*.
`init()` runs once per Spark task (`DocumentParser`'s contract says so, and
`UnstructuredFileRecordBuilder` calls it on a transient field), so on one
executor JVM:
- task 1: `compareAndSet` returns true, probes, throws if the modules are
missing
- task 2+: `compareAndSet` returns false, short-circuits, never probes,
never throws
Spark then retries the failed task in the same JVM, the retry short-circuits
and succeeds, and the job exits 0 with `parse_status=EMPTY` on every row. That
is the exact outcome the probe was added to prevent.
Replaced with a memoized *result* behind `hasParserModules()`, so the probe
still runs at most once per JVM but every task fails when the answer is
negative. Covered by
`TestUnstructuredIngestHardening#testMissingParserModulesFailEveryTaskNotOnlyTheFirst`;
I confirmed it fails against the old latch semantics before landing the fix,
on the assertion for task 2.
##########
packaging/hudi-utilities-bundle/pom.xml:
##########
@@ -97,6 +97,45 @@
<include>org.apache.hudi:hudi-spark-client</include>
<include>org.apache.hudi:hudi-utilities_${scala.binary.version}</include>
<include>org.apache.tika:tika-core</include>
+ <!-- Apache Tika document parsers, for
UnstructuredFileDFSSource -->
+ <include>com.adobe.xmp:*</include>
Review Comment:
There is such a dependency and it was already declared:
`tika-parsers-standard-package` in `hudi-utilities/pom.xml` pulls all of these
transitively. The explicit includes were needed for an unrelated reason, which
is that the shade plugin's `artifactSet` is an allowlist, so anything not named
is dropped from the jar no matter how it reached the dependency tree.
So the includes were not redundant, but the objection holds on other
grounds, and the strongest one is not size: the `commons-compress` relocation
this required applied to **everything** in the utilities bundle, not only to
Tika's POI. On top of ~50 MB and ~30 third-party licences added to the flagship
bundle for a feature most of its users do not touch.
Moved to `packaging/hudi-tika-bundle`, following the existing
`hudi-aws-bundle` / `hudi-gcp-bundle` precedent for optional capabilities. The
relocation moves with it, so its blast radius is one optional artifact.
`tika-core` stays in the utilities bundle because `hudi-utilities` compiles
against it and nothing else does; the new bundle excludes it so passing both
does not put two copies on the classpath.
Verified on the built jars:
| check | result |
|---|---|
| hudi-utilities-bundle | 95 MB, 0 parser classes, tika-core still present |
| hudi-tika-bundle | 52 MB, tika-core correctly excluded |
| commons-compress | 612 relocated entries, 0 unrelocated |
| POI `ZipPackage` / Tika `PackageParser` | 14 / 51 relocated refs, 0
unrelocated |
| relocated `ArchiveStreamFactory` | declares `detect()`, so the 1.27.1 pin
took effect |
| merged service registry | 85 parser implementations |
`tika-parsers-standard-package` also goes back to test scope in
`hudi-utilities`: only `tika-core` is compiled against, and the parser modules
are discovered through Tika's ServiceLoader at runtime. Confirmed with a clean
compile.
One note on "ask users to pass that", since it shapes what the artifact has
to be: `--packages` cannot be the mechanism. Spark ships commons-compress 1.23,
POI needs 1.24 or newer, and user jars do not outrank Spark's own on the
classpath. A shaded bundle carrying the relocation is what actually works,
which is why this is a bundle rather than a documented coordinate.
--
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]
