Hi All, Thanks for the feedback. A few thoughts:
-- *The part I am still not comfortable with is using **publishToMavenLocal** in a required PR test...* Today we already have other CI tests that run publishToMavenLocal. Since we're not testing the publish functionality itself, and the contents published to the local Maven repository are deterministic for a given PR, the side effects should be minimal. In practice, the local Maven repository should contain the same artifacts across all CI jobs for the same PR. *-- My preference would be to keep the JUnit/Gradle test for the bundle jar and Spark catalog behavior...* I think we should keep the --packages test because it is one of the standard and well-established ways of launching Spark applications. It's an important user workflow, so I think it's worth verifying that it continues to work correctly. For the --jars path, I don't think we need a separate way of testing. We already start the environment once and run the full regression suite in that environment. Those tests provide value beyond functional verification—they also validate that the Spark client can be packaged, launched, and used successfully in an environment that closely resembles a real deployment. *-- I am definitely not against Spark 4 coverage. I just think the required PR tests should be matched to the specific failure modes...* I agree there is room for improvement in terms of code reuse. The primary difference between the Spark 3 and Spark 4 test environments should just be the Docker definition. Most of the test cases are fairly basic and should be reusable across both versions. We can likely refactor the test framework so that the same test suite runs against different Spark environments with minimal duplication. Best Regards, Yun On Thu, Jul 16, 2026 at 4:46 PM Dmitri Bourlatchkov <[email protected]> wrote: > Hi All, > > Robert's proposal looks good to me... except that I'm not sure what "narrow > Spark launcher test for `--packages`" means :) but I'd be fine with just > reviewing that part in a PR. > > Cheers, > Dmitri. > > On Mon, Jul 13, 2026 at 10:52 AM Robert Stupp <[email protected]> wrote: > > > Hi Yong, > > > > Yun's example helps, because it does point to a real failure mode: > Spark's > > `--packages` path can catch dependency-resolution problems that a simple > > `--jars` or bundle-jar test might miss. > > > > But I am not sure that leads all the way to "copy the current > Docker-based > > regtests for Spark 4". > > > > I think there are a few different concerns mixed together here: > > > > * Is the generated POM / dependency metadata correct? > > * Does the bundle/shadow jar load and work in a Spark-like runtime? > > * Does Spark's real `--packages` path resolve and launch the client > > correctly? > > > > The first one can be checked by inspecting the generated metadata and > > resolved > > dependency graph. > > The second one seems like a good fit for the JUnit/Gradle test. > > The third one may still need a real Spark launcher smoke test, but I > think > > that > > test should be very small and explicit. > > > > The part I am still not comfortable with is using `publishToMavenLocal` > in > > a > > required PR test. > > It mutates global `~/.m2`, which is bad for project isolation, and Maven > > local > > is not the same as Maven Central anyway. > > So even if the test passes there, it is not exactly proving the same > thing > > users > > will see when they resolve released artifacts. > > > > My preference would be to keep the JUnit/Gradle test for the bundle jar > and > > Spark catalog behavior, add direct metadata/dependency checks if needed, > > and > > only keep a narrow Spark launcher test for `--packages` if we agree that > > this > > specific path needs PR coverage. > > > > I am definitely not against Spark 4 coverage. > > I just think the required PR tests should be matched to the specific > > failure > > modes we want to catch, rather than copying the whole Docker regtest > setup > > by > > default. > > > > Robert > > > > > > On Sat, Jul 11, 2026 at 8:37 PM Yong Zheng <[email protected]> wrote: > > > > > Thanks for the sharing the details as well as what happened in the > past. > > > in this case, any concern with adding docker based regression test to > > spark > > > 4 for now until we have a better solution? > > > > > > @ Dmitri and Robert > > > > > > Thanks, > > > Yong Zheng > > > > > > On 2026/07/07 01:36:48 yun zou wrote: > > > > Hi, > > > > > > > > Thanks for laying out your reasoning. I agree with several of your > > goals, > > > > particularly that every required CI test should have a clear purpose > > and > > > > that we should avoid redundant coverage. However, I don't think the > > > > proposed JUnit/Gradle test is a complete replacement for the current > > > Spark > > > > regression test. > > > > > > > > First, I think the "~2m30s" point is being interpreted a bit > > differently > > > > than intended. It wasn't meant to justify keeping a heavy test simply > > > > because it's "only" a few minutes. Rather, it was in response to the > > > > characterization that the current Spark regression test is > > prohibitively > > > > expensive. The runtime is relatively modest for the coverage it > > provides. > > > > > > > > Second, I think the current regression test is validating two > different > > > > things: > > > > > > > > - the Polaris service behaves correctly; and > > > > - the Spark client can be packaged, launched, and used > successfully > > > in a > > > > realistic environment. > > > > > > > > Those have different goals and different failure modes. For example, > > > > differences in storage backends or I/O implementations are important > > for > > > > the service-side tests, but they aren't really the focus of > validating > > > the > > > > Spark client. I'd actually favor separating those concerns into two > > > > different CI jobs rather than replacing the Spark client regression > > test > > > > entirely. > > > > > > > > Regarding --packages, we added that coverage for a concrete reason. > > While > > > > our documentation primarily demonstrates using --jars, both --jars > and > > > > --packages are well-established ways of launching Spark applications, > > and > > > > users commonly use --packages as an alternative. > > > > > > > > Historically, we started with only the --jars regression test. We > later > > > > added --packages support because we encountered a real regression > that > > > the > > > > --jars test did not catch: using --packages pulled in the transitive > > > > dependencies of polaris-core, which conflicted with the version of > Avro > > > > required by Spark. That failure mode only appeared when Spark > performed > > > > dependency resolution in the same way users do. > > > > > > > > This is also why I'm not convinced a JUnit test is sufficient. To > > achieve > > > > equivalent coverage, we'd effectively need to mimic how Spark > resolves > > > both > > > > --jars and --packages. That means introducing another layer of test > > logic > > > > that we'd have to maintain, and one that necessarily depends on > Spark's > > > > implementation. I'd rather exercise Spark's actual launcher and > > > dependency > > > > resolution than maintain our own approximation of that behavior. > > > > > > > > So I agree that we should make the responsibilities of the regression > > > tests > > > > clearer, and I'm supportive of separating the service-focused and > Spark > > > > client-focused tests. Where I disagree is that the Docker-based Spark > > > > regression test is merely an end-to-end duplicate. It validates > > packaging > > > > and launch behavior that has already caught at least one real > > regression > > > > and does so by exercising the same code paths our users rely on. > > > > > > > > Best Regards, > > > > Yun > > > > > > > > On Sat, Jul 4, 2026 at 4:30 PM Yong Zheng <[email protected]> wrote: > > > > > > > > > Hello, > > > > > > > > > > I think one example provided in the above reference ticket is > Russell > > > ran > > > > > into some issue in the past with this thus the strong preference > over > > > > > docker based testing. @Russell, is this something you can provide > > more > > > > > insights with? > > > > > > > > > > Thanks, > > > > > Yong Zheng > > > > > > > > > > > > > > > On 2026/06/30 16:55:42 Dmitri Bourlatchkov wrote: > > > > > > Hi All, > > > > > > > > > > > > +1 to Robert's points. > > > > > > > > > > > > Testing just the "fat" client jar in CI looks sufficient to me. > > This > > > jar > > > > > > should expose the same range of class-loading issues that may > occur > > > with > > > > > > the "thin" jar with dependencies resolved via Maven/Ivy. > > > > > > > > > > > > Additionally, I think Gradle-based tests are much simpler to > debug > > > and > > > > > > evolve than Docker-based tests. > > > > > > > > > > > > Cheers, > > > > > > Dmitri. > > > > > > > > > > > > On Tue, Jun 30, 2026 at 6:25 AM Robert Stupp <[email protected]> > > wrote: > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > I think the “this only takes ~2m30s” argument is a bit > > distracting. > > > > > > > > > > > > > > The question should not be whether one CI section is currently > > > small > > > > > > > compared to other CI sections. > > > > > > > The question should be: what concrete failure mode does this > test > > > catch > > > > > > > that we cannot catch with a cheaper and more targeted test? > > > > > > > > > > > > > > GitHub runner time is still a shared ASF resource. > > > > > > > Even a few minutes matter when they run on many PRs, retries, > > > > > main/release > > > > > > > branches, and then get copied again for Spark 4 or future Spark > > > > > versions. > > > > > > > So I think every required PR test should have a clear purpose > > and a > > > > > clear > > > > > > > failure mode it protects against. > > > > > > > > > > > > > > For the Spark plugin regtest, I am still missing that concrete > > > > > > > justification. > > > > > > > > > > > > > > If the concern is the bundle jar, then I agree we should test > > that > > > the > > > > > > > bundle jar loads in an isolated Spark-like runtime and can > > > create/use > > > > > the > > > > > > > Polaris catalog. > > > > > > > That seems valuable, and the JUnit/Gradle test looks like a > good > > > fit > > > > > for > > > > > > > that. > > > > > > > > > > > > > > If the concern is `--packages` / Maven resolution, I am less > > > convinced > > > > > this > > > > > > > belongs in required PR CI. > > > > > > > Polaris appears to direct users to the packaged Spark client > > > artifacts, > > > > > > > especially the bundle jar, for example on the 1.5.0 downloads > > page. > > > > > > > Testing Maven/Ivy resolution through `publishToMavenLocal` also > > has > > > > > real > > > > > > > costs: it mutates the developer's global `~/.m2`, interacts > badly > > > with > > > > > > > project isolation, and is not great for build cacheability. > > > > > > > > > > > > > > Also, the risk of “broken generated POM metadata” seems very > low. > > > > > > > If we really care about that, we can check the publication > > metadata > > > > > > > directly without launching a Docker/Spark workflow. > > > > > > > > > > > > > > So my concrete question is: > > > > > > > Has the Docker-based Spark plugin regtest caught specific > > > regressions > > > > > that > > > > > > > the proposed isolated JUnit/Gradle test would not have caught? > > > > > > > > > > > > > > Examples would help a lot here: broken dependency metadata, a > > real > > > > > > > `spark-submit --packages` failure, a bundle/classpath issue, or > > > some > > > > > > > launcher behavior that only the Docker test exposed. > > > > > > > Without that evidence, “it is closer to the user workflow” > feels > > > too > > > > > broad > > > > > > > to justify keeping it as a required PR gate. > > > > > > > > > > > > > > My preference would be: > > > > > > > > > > > > > > * keep required PR CI focused on targeted tests for the bundle > > jar > > > and > > > > > > > Spark > > > > > > > catalog behavior; > > > > > > > * avoid `publishToMavenLocal` and global `~/.m2` mutation in > > > normal PR > > > > > > > tests; > > > > > > > * if people still want full shell/Docker coverage, run it > > > periodically > > > > > or > > > > > > > as a > > > > > > > manual workflow until we have evidence that it catches unique > > > > > > > regressions. > > > > > > > > > > > > > > That gives us Spark 4 coverage without making Docker-based > > > end-to-end > > > > > > > testing the default answer for every Spark version. > > > > > > > > > > > > > > Robert > > > > > > > > > > > > > > On Tue, Jun 30, 2026 at 1:27 AM Yufei Gu <[email protected] > > > > > wrote: > > > > > > > > > > > > > > > Thanks for raising this, Yong! I agree that we need tests for > > > Spark > > > > > 4. > > > > > > > > > > > > > > > > I agree with what Yun said here. > > > > > > > > > > > > > > > > To add to that, the current regression tests against > > MinIO/RustFS > > > > > cover > > > > > > > > both the Spark Plugin Regression Test and the top level > > > Regression > > > > > Test. > > > > > > > > These used to be separate CI workflows(merged in PR 3625), > and > > I > > > > > think we > > > > > > > > should keep them separate. > > > > > > > > > > > > > > > > The Spark Plugin Regression Test does not need to connect to > a > > > > > storage > > > > > > > > system such as S3, MinIO, or RustFS. It primarily serves as a > > > smoke > > > > > test > > > > > > > to > > > > > > > > verify the Polaris packaging and Spark deployment. I think we > > > should > > > > > > > > restore the previous setup where these workflows are > separated. > > > That > > > > > > > would > > > > > > > > also reduce the overall CI duration, since they can run in > > > parallel. > > > > > > > > > > > > > > > > [image: Screenshot 2026-06-29 at 4.15.57 PM.png] > > > > > > > > [image: Screenshot 2026-06-29 at 4.16.14 PM.png] > > > > > > > > > > > > > > > > Yufei > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jun 29, 2026 at 4:07 PM yun zou < > > > [email protected]> > > > > > > > > wrote: > > > > > > > > > > > > > > > >> Hi Yong Zheng, > > > > > > > >> > > > > > > > >> Thanks for bringing this up! In short, I don't think it's > > worth > > > the > > > > > > > effort > > > > > > > >> to make this conversion at the moment for the following > > reasons: > > > > > > > >> > > > > > > > >> 1. *It doesn't meaningfully improve CI time.* I think you > > > > > mentioned > > > > > > > >> this > > > > > > > >> in the thread as well. Looking at one CI run as an > example > > ( > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > https://github.com/apache/polaris/actions/runs/24255532169/job/70826005994 > > > > > > > >> ), > > > > > > > >> the Spark Regression Test section only takes about *2m > > 35s*. > > > > > Even if > > > > > > > we > > > > > > > >> add another Spark 4.x regression test, I don't think it > > would > > > > > > > >> significantly > > > > > > > >> increase the overall CI time—probably just another 2–3 > > > minutes. > > > > > The > > > > > > > >> Runtime > > > > > > > >> Service tests are still the slowest part of the pipeline, > > and > > > > > their > > > > > > > >> execution time is likely to continue growing. > > > > > > > >> 2. *The regression tests provide a high level of > confidence > > > in > > > > > > > >> correctness.* They remain the tests that most closely > > > resemble > > > > > our > > > > > > > >> customers' actual environments, making them our last line > > of > > > > > defense > > > > > > > >> against regressions. That gives them significant value. > > > Rather > > > > > than > > > > > > > >> spending effort trying to build simulations that provide > > > similar > > > > > > > >> coverage, > > > > > > > >> I think it's better to keep these regression tests in > place > > > since > > > > > > > they > > > > > > > >> validate the real end-to-end behavior. > > > > > > > >> > > > > > > > >> Those are my thoughts, but I'm happy to discuss further if > you > > > see > > > > > > > >> additional benefits that I'm missing. > > > > > > > >> > > > > > > > >> Best Regards, > > > > > > > >> Yun > > > > > > > >> > > > > > > > >> On Sun, Jun 28, 2026 at 8:47 PM Yong Zheng < > [email protected] > > > > > > > > wrote: > > > > > > > >> > > > > > > > >> > Hello, > > > > > > > >> > > > > > > > > >> > Follow-up to the regtest thread ( > > > > > > > >> > > > > https://lists.apache.org/thread/4bx31cfbcqfxzgpsddvc9kcfbn9l093y) > > > > > and > > > > > > > >> > current PR (https://github.com/apache/polaris/pull/4588). > > > > > > > >> > > > > > > > > >> > Currently we support both Spark 3 ( > > > > > > > >> > > > > https://github.com/apache/polaris/tree/main/plugins/spark/v3.5) > > > > > and > > > > > > > 4 ( > > > > > > > >> > > > > https://github.com/apache/polaris/tree/main/plugins/spark/v4.0) > > > > > for > > > > > > > >> > Polaris spark client, however, only spark 3 has regtests. > > > There > > > > > was a > > > > > > > >> > concern with potentially increasing CI time, however, this > > > later > > > > > got > > > > > > > >> proved > > > > > > > >> > to be not the case as "moving > > > > > > > >> > regtests to integration tests would not necessarily save > > > time. In > > > > > > > fact, > > > > > > > >> it > > > > > > > >> > could potentially increase overall CI duration, since the > > > longest > > > > > > > >> running > > > > > > > >> > workflows are currently not the regtests". > > > > > > > >> > > > > > > > > >> > Before we can finalize the testing strategy for Polaris > > spark > > > > > client, > > > > > > > we > > > > > > > >> > need to decide if we want to proceed with the conversion > > (from > > > > > docker > > > > > > > >> based > > > > > > > >> > to JUnit based). The lack of regtests for spark 4 can > > > potentially > > > > > > > cause > > > > > > > >> > regression issues later. > > > > > > > >> > > > > > > > > >> > Also, as we are using JUnit, we can't trigger a actual > > > > > 'spark-shell > > > > > > > >> xxxx' > > > > > > > >> > to simulate the actual `--packages` and `--jars`. > > > > > > > >> > > > > > > > > >> > However, we can kind get them working by using > > > `URLClassLoader` > > > > > for > > > > > > > >> > `--jars` and `SparkSubmitUtils.resolvedMavenCoordinates` > for > > > > > > > >> `--packages`. > > > > > > > >> > The catch here is to be able to use `--packages`, we will > > > need to > > > > > > > >> > `publishToMavenLocal` (which is project-isolation > violation, > > > as it > > > > > > > will > > > > > > > >> try > > > > > > > >> > to modify `~/.m2`). The suggest is to drop this test and > > only > > > > > handle > > > > > > > >> bundle > > > > > > > >> > jar via `URLClassLoader`. > > > > > > > >> > > > > > > > > >> > I am wondering how team would like to proceed as we can't > > > leave > > > > > spark > > > > > > > 4 > > > > > > > >> > out there without proper JUnit for a long period of time. > > > > > > > >> > > > > > > > > >> > Thanks, > > > > > > > >> > Yong Zheng > > > > > > > >> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
