Repository navigation
test: tag the Azure ABFS suites with AzureTest - #6942
Conversation
AbfsAuthParitySuite, AbfsAuthResolverSuite and AbfsScanFallbackSuite resolve ABFS authentication through the hadoop-azure on the test classpath, so they pass only when that hadoop-azure matches the Hadoop the build runs on. A build against a different Hadoop line, for example a vendor Hadoop with no matching hadoop-azure pin, had no way to skip them: the root pom hard-codes tagsToExclude, so -DtagsToExclude on the command line was ignored. Add an org.apache.comet.tags.AzureTest tag, annotate the three suites with it, and append a tagsToExclude property (empty by default) to the scalatest-maven-plugin exclusions so -DtagsToExclude=org.apache.comet.tags.AzureTest skips them. CI keeps running them, since every Spark profile pins a matching hadoop-azure.
|
@snmvaughan FYI |
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full six-file diff from 474b5ece841a7fc6dff37ae04bd6787e654d7be7 to 27a37490741e5bb1b7e273e71962671bc2e7e1f8. The PR is not a draft. No introduced P1/P2 issues found within this review. Existing discussion contains no unresolved substantive concerns.
Summary
- Prior state and problem: The ABFS suites depend on compatible Hadoop/Azure artifacts, while hard-coded Maven exclusions prevented selectively skipping them through
-DtagsToExclude. - Design approach: Add the runtime-retained
AzureTestannotation to three suites and append configurable exclusions to the existing ScalaTest configuration. - Correctness: ScalaTest 3.2.16 source confirms that class annotations tag every test. The Maven plugin’s parsing handles the default trailing comma.
IntegrationTestSuiteremains excluded, and exact-head CI confirms the Azure suites still execute by default. - Compatibility analysis: Checked Spark test-base sources for the pinned versions
3.4.3,3.5.9,4.0.4,4.1.3, and4.2.0. They retain ScalaTest’s class-tag behavior. No expression, operator, authentication implementation, or query semantics change. - Key design decisions: Azure exclusions are opt-in, integration exclusions remain unconditional, and all three suites remain registered in both workflow lists.
- Implementation sketch: The root POM adds an empty property and interpolates it into plugin configuration. A Java annotation, three suite annotations, and contributor documentation complete the change.
- Performance: The change affects test selection only and adds no production execution overhead. No performance issue meeting the reporting bar was identified.
- Design: Standard ScalaTest tagging directly addresses the selective-skip requirement without changing dependency resolution or authentication behavior.
- Abstraction & complexity: One annotation follows the existing
IntegrationTestSuitepattern. No additional framework or version-dependent abstraction is introduced. - Behavioral changes worth calling out:
-DtagsToExclude=org.apache.comet.tags.AzureTestnow requests exclusion of these suites. Compared withbranch-1.1at18732aa2150b52e1b456fdb6317fee5947edfb68, this is an intended test-selection capability. The Azure suites themselves were already added before this PR. Default production behavior is unchanged. - Suggested improvements: No additional issue or improvement meeting the P1/P2 reporting bar was identified.
Routed skill: .ai/skills/review-comet-pr/SKILL.md. No sibling area skill applies to this build/test-tag change. Repository guidance, existing reviews, conversation comments, inline comments, and review threads were checked.
Exact-head CI: 26 checks passed and 18 were skipped, with no failures. Required Checks passed. The Linux Spark 4.1 execution log confirms 184 passing tests across the three Azure suites and two expected Hadoop-version-gated cancellations. Spark SQL, Iceberg, and macOS jobs were skipped. CI run.
Validation limits: dev/ci/check-suites.py and the base-relative git diff --check passed locally. JVM/native tests were not rerun locally because this checkout lacks compiled artifacts, cached Maven dependencies, and javac. Exclusion with mismatched Hadoop artifacts was assessed through source inspection, not independently reproduced. Project code was unchanged.
andygrove
left a comment
There was a problem hiding this comment.
Nit, optional: I tried this on a mismatched classpath, -Pspark-3.5 -Dhadoop-azure.version=3.4.2, which puts hadoop-azure 3.4.2 on hadoop-client-api 3.3.4. With -DtagsToExclude=org.apache.comet.tags.AzureTest the three suites were skipped and the build passed, so the mechanism works. Without the flag, though, only one test failed: FileSystem.newInstance agrees with the resolver on every case in AbfsAuthParitySuite. The other 139 tests in that suite, all of AbfsAuthResolverSuite and the ABFS tests in NativeConfigSuite passed. That matches the bytecode. AzureBlobFileSystem 3.4.2 links against BackReference, Options$OpenFileOptions, RateLimitingFactory and some shaded Guava classes that hadoop-client-api 3.3.4 doesn't have, while AbfsConfiguration, which is all the resolver reaches, needs nothing newer.
Which build did you see AbfsAuthResolverSuite and AbfsScanFallbackSuite fail on? If it was only the cross-check, could the tag go on that one test instead, with a ScalaTest Tag of the same name passed to test(...)? A build that needs the flag would then keep running the resolver cases, and the resolver is the code that decides which credentials reach native. If the resolver suite really does fail on your build, I'd expect NativeConfigSuite to fail there too, since its ABFS tests go through the same AbfsAuthResolver.resolve and it isn't tagged. Either way the sentence in development.md that says these suites pass only when hadoop-azure matches would need to follow.
On a mismatched classpath only the FileSystem.newInstance cross-check in AbfsAuthParitySuite fails. AzureBlobFileSystem links against Hadoop classes an older Hadoop lacks, while the resolver reaches nothing newer than AbfsConfiguration, so the resolver cases, AbfsAuthResolverSuite and AbfsScanFallbackSuite still pass there. Tag just that test, with a ScalaTest Tag of the same name, so a build that passes -DtagsToExclude=org.apache.comet.tags.AzureTest keeps running the resolver cases.
|
@andygrove thanks for trying it on a mismatched classpath. You're right. The build I hit was hadoop-azure 3.4.2 on Hadoop 3.3.4, and there the only failure was the I pushed the change you suggested:
Locally on the default profile, |
Which issue does this PR close?
No issue is filed for this. It is a small follow-up to #6059.
Rationale for this change
AbfsAuthParitySuite,AbfsAuthResolverSuiteandAbfsScanFallbackSuiteresolve ABFS authentication through thehadoop-azureon the test classpath. They pass only when thathadoop-azurematches the Hadoop the build runs on. Every Spark profile in this repository pins a matchinghadoop-azure.version, so CI is fine.A build against a different Hadoop line, such as a vendor Hadoop or an overridden
hadoop.versionwithout a matchinghadoop-azure.version, fails these suites withNoClassDefFoundError. There was no way to skip them, because the root pom hard-codes the scalatesttagsToExclude, so-DtagsToExcludeon the command line was ignored.What changes are included in this PR?
org.apache.comet.tags.AzureTestscalatest tag, applied to the three Azure suites.tagsToExcludeproperty in the root pom, empty by default, appended to thescalatest-maven-pluginexclusions.-DtagsToExclude=org.apache.comet.tags.AzureTestnow skips the Azure suites.IntegrationTestSuitestays excluded either way.Nothing changes by default. The suites stay in both workflow lists and still run in CI.
How are these changes tested?
I ran these locally on the default profile with JDK 17, using
-Dsuites=org.apache.comet.objectstore.AbfsAuthParitySuite,org.apache.comet.objectstore.AbfsAuthResolverSuite:-DtagsToExclude=org.apache.comet.tags.AzureTest: 0 tests run.I did not run
AbfsScanFallbackSuitelocally, because it needs the native library.dev/ci/check-suites.pypasses.