danhuawang commented on PR #13266:
URL: https://github.com/apache/gravitino/pull/13266#issuecomment-5711403322
Thanks for adding first-party connector init-container images. The
Gradle-based version discovery is a good fit for OSS branches. A few issues
should be addressed before merge:
### Must fix
1. **`IMAGE_VERSION` is stale.** All three Dockerfiles default to
`1.3.0-SNAPSHOT`, but `main` is `2.0.0-SNAPSHOT` (`gradle.properties`). Please
inject the version at build time (e.g. `--build-arg` from `gradle.properties`)
instead of hard-coding it. This value is also the Red Hat `version` LABEL.
2. **Images are not wired into the existing Docker build path.** Other
images are built via `dev/docker/build-docker.sh --type ...` (dependency
script, then `docker buildx`). This PR does not update that script, CI, or
publish docs, so `apache/gravitino-{trino,flink,spark}-connector` will not be
produced by the current release workflow. Please add `--type` entries (and a
short build README) for:
- `flink-connectors`
- `spark-connectors`
- `trino-connectors`
3. **LICENSE / NOTICE in the image are not appropriate for a binary
distribution.** The three copied files are essentially the source-tree
LICENSE/NOTICE:
- they reference `web/LICENSE` and `web/web/NOTICE`, which are not in the
image
- they list many source-file attributions unrelated to the connector jars
- README/Dockerfile claim the image is “Apache-2.0 only”, but shaded
runtime jars include third-party classes
Prefer copying the repo-root LICENSE/NOTICE at build time (avoid three
drifting copies), and include licenses that match what is actually shaded into
the jars.
### Suggestions
4. **`copy-connector.sh` exits 0 when `/target` is missing.** That is
convenient for `docker run --rm <image>` to list versions, but a Kubernetes
init container that forgot the volume will succeed and the engine will start
with an empty directory. Consider requiring an explicit list mode
(`LIST_VERSIONS=true`) and `exit 1` when `/target` is absent on the install
path.
5. **Trino `resolve_version_range` + `set -e`.**
`VERSION_RANGE="$(resolve_version_range ...)"` will abort on `return 1` in Bash
4.4+ before the “not supported” message. Return empty with status 0, or use `||
true`.
6. **Gradle discovery swallows errors:** `./gradlew -q projects
2>/dev/null`. If Gradle fails, the script only reports “no modules found”. Keep
stderr. For Trino, copy only the bands produced by this run; do not glob all
leftover `distribution/gravitino-trino-connector-*` dirs from previous builds.
7. **Spark default `SCALA_VERSION=2.12`.** Spark 4.0 on `main` is 2.13-only.
`SPARK_VERSION=4.0` with the default Scala will fail. Default 2.13 for 4.x, or
fail with a clearer message.
8. **UBI / OpenShift nits:** `chgrp`/`chmod g=u` is only applied to
`/connectors` (not `/licenses`). `USER 1000` has no passwd entry, which
certification often wants. Comments say “Certified Image”; this repo has no
certification pipeline — “UBI 10-based, certification-oriented labels” would be
more accurate.
9. **Testing.** Flink was built end-to-end; Trino/Spark are mostly `bash
-n`. Please assert that discovered dirs are non-empty and that the default
`FLINK_VERSION` / `SPARK_VERSION` / `TRINO_VERSION` exist on the branch being
built.
### Looks good
- Runtime resolution from `/connectors` is better than a hard-coded matrix
across `main` / `branch-1.3`.
- Spark 4.0 locked-Scala detection matches
`spark-connector/v4.0/spark-runtime/build.gradle.kts`.
- Omitting JDBC drivers from the Trino image matches Trino plugin classpath
isolation.
- Excluding `*-empty.jar` for Flink/Spark is correct.
- Install notes (Flink `usrlib`, do not overlay `/opt/spark/jars`, Trino
plugin dir must be `gravitino`) are useful.
--
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]
