andygrove opened a new pull request, #5892: URL: https://github.com/apache/datafusion-comet/pull/5892
## Which issue does this PR close? Closes #5023. ## Rationale for this change This is the alternative @comphead asked for on #5890: rather than vendoring a patched `libhdfs` into Comet to fix the flake, stop running the one suite that triggers it in CI and run it by hand instead. I think that is the right call. The crash is real and the mechanism is understood, but the fix costs us a copy of another project's C in our tree that someone then has to maintain, and HDFS is not a direction we are investing in. The mechanism, briefly, so the tradeoff is on the record. `libhdfs` registers a pthread thread-local destructor that detaches the current thread from the JVM whenever it finds a cached `JNIEnv`, regardless of who attached the thread. Comet attaches its own Tokio workers and detaches them in `on_thread_stop`, so on a worker that has touched HDFS the `JNIEnv` is freed first and the destructor then dereferences it and jumps to address zero. That is [HDFS-16021](https://issues.apache.org/jira/browse/HDFS-16021), still open upstream. It fires when the pooled thread exits, which is usually minutes after the HDFS read, in whatever suite happens to be running by then — that is why it has been reading as a random `[scans]` failure rather than an HDFS one, and it has cost us re-runs on several branches. To be clear about what this does and does not do: it removes CI's exposure, it does not fix the bug. An HDFS user running Comet can still hit it. The docs changes below are there so that is not a surprise. ## What changes are included in this PR? `ParquetReadFromFakeHadoopFsSuite` moves onto the existing "manual test suite" convention already used by `ParquetReadFromS3Suite`, `IcebergReadFromS3Suite` and `CometS3CredentialBridgeSuite`: drop it from the `scans` bucket in both `pr_build_linux.yml` and `pr_build_macos.yml`, and add it to the `ignore_list` in `dev/ci/check-suites.py` so preflight stops requiring it. The suite itself is unchanged apart from a scaladoc recording why it is out of CI and the command to run it. It is the only suite that loads `libhdfs`. `CometScanSchemeFallbackSuite` also exercises an `hdfs://` path, but it only applies `CometScanRule` to the plan and never executes, so it never loads the library — meaning the planner-side half of the coverage, that an `hdfs://` scan is still claimed natively instead of silently falling back, stays in CI. The comment in `CometScanRule` that pointed only at the suite being removed is updated to say which half lives where. The Rust HDFS tests in `parquet_writer.rs` are already `#[ignore]`d as needing a live cluster. On the docs side, the HDFS section of the data sources guide gets a warning that support is experimental, is not covered by CI, and can crash the JVM, with links to HDFS-16021 and #5023, plus the manual test command in the local-development section. The contributor guide gains a short note on the `check-suites.py` ignore list, which was previously undocumented. I also fixed something I ran into while checking which tests touch HDFS: the Iceberg guide lists "Hadoop Distributed File System (HDFS)" under supported storage for the native Iceberg reader, and that is not true. `storage_factory_for` in `iceberg_common.rs` has arms for `file`, `memory`, `gs`, `oss` and the s3 family only, `CometScanRule.icebergReadableSchemes` mirrors it, and the roadmap already says HDFS-backed Iceberg tables are unsupported. The guide now says they fall back to Spark, and the storage list picks up `gs` and `oss`, which were supported but unlisted. ## How are these changes tested? `dev/ci/check-suites.py` and `dev/ci/check-ci-config.py` both pass, which is the check that would have failed if the workflow edit and the ignore-list edit had got out of sync. The point of the change is that the suite still works when run by hand, so I ran the documented command against a debug build on macOS/arm64 with Spark 4.1: ``` ./mvnw test -Dtest=none -Dsuites="org.apache.comet.parquet.ParquetReadFromFakeHadoopFsSuite" ParquetReadFromFakeHadoopFsSuite: - native scan on fake fs Tests: succeeded 1, failed 0, canceled 0, ignored 0, pending 0 All tests passed. ``` Note the test was not canceled, so `hdfs-opendal` was present in the build and the `libhdfs` path really was exercised rather than skipped by the `assume`. `./mvnw test-compile` and scalastyle are clean, `spotless:apply` is a no-op on the result, and prettier is clean on both edited docs. If we go with this, #5890 should close unmerged. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
