tkaymak commented on PR #38233:
URL: https://github.com/apache/beam/pull/38233#issuecomment-4269815902
Thanks @gemini-code-assist for the careful review.
**Comment 1 (lexicographic version compare): addressed.** Switched to
index-based ordering with whitespace trim and an explicit `GradleException` if
`spark_major` isn't listed in `spark_versions` — same shape as your suggestion:
```groovy
def all_versions = spark_versions.split(",").collect { it.trim() }
def spark_major_index = all_versions.indexOf(spark_major)
if (spark_major_index < 0) {
throw new GradleException(
"spark_major='${spark_major}' is not listed in
spark_versions='${spark_versions}' " +
"(see root gradle.properties).")
}
def previous_versions = spark_major_index > 0 ? all_versions.subList(0,
spark_major_index) : []
```
**Comment 2 (`use_override`): addressed.** Now `def use_override =
(spark_major_index > 0)`.
**Comment 3 (always include `./src` for the base version): respectfully
declining.** This deliberately mirrors `runners/flink/flink_runner.gradle`,
where the lowest supported version (`runners/flink/1.17/`) likewise does not
add its own `./src` to `srcDirs` — the base version *is* the shared source.
Adding an always-empty `./src/main/java` to the base would (a) diverge from the
established Flink pattern reviewers will compare against and (b) silently
encourage placing code in `runners/spark/3/src/` that the override layering
then can't see from higher versions, which is the exact failure mode this
refactor is trying to prevent. If a future Spark 3-only file is ever needed,
the cleanest move is to bump the base out from under it (or add the srcDir
then), not to pre-create the seam.
Force-pushed the updated commit; please take another look.
--
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]
