numinnex commented on code in PR #3658:
URL: https://github.com/apache/iggy/pull/3658#discussion_r3691121109
##########
.github/actions/utils/docker-buildx/action.yml:
##########
@@ -342,6 +346,7 @@ runs:
with:
context: ${{ steps.ctx.outputs.context }}
file: ${{ steps.config.outputs.dockerfile }}
+ target: ${{ inputs.target }}
Review Comment:
`target` now changes the build graph, but the buildx cache ref is still only
component- and arch-scoped, so the two flavors fight over one cache entry.
The cache composition step above builds `cache-to` / `cache-from` as
`type=registry,ref=${img}:buildcache-${arch},mode=max` (and the shared
`${shared_deps}:buildcache-${arch}`) with no flavor component. Since
`docker_matrix` is now component x flavor x platform, the fat and slim jobs for
the same arch run concurrently and both push `mode=max` to
`apache/iggy-connect:buildcache-amd64`. The cache index is last-writer-wins, so
each run one flavor's cache export is effectively lost, and `cache-from` may
pull an index describing the other flavor's graph.
Layer integrity is fine (BuildKit verifies digests), but the fat build is
the expensive one and is exactly the one that needs a warm cache.
`${img}:latest` in `cache-from` is also the fat image under the new tag scheme,
which the slim build will now pull as inline cache.
Suggestion: thread the flavor into the cache ref, e.g. pass the flavor
suffix into this action and use `${img}:buildcache${suffix}-${arch}`, so each
flavor keeps its own cache the same way it now keeps its own digest artifacts.
##########
.github/workflows/_build_rust_artifacts.yml:
##########
@@ -238,13 +251,16 @@ jobs:
outdir="dist/${target}"
mkdir -p "${outdir}"
- plugins="${{ inputs.connector_plugins }}"
- IFS=',' read -ra pkgs <<< "$plugins"
+ IFS=',' read -ra pkgs <<< "$CONNECTOR_PLUGINS"
for pkg in "${pkgs[@]}"; do
lib_name="$(echo "$pkg" | xargs)"
[[ -z "$lib_name" ]] && continue
so_file="target/${target}/release/lib${lib_name}.so"
- [[ -f "$so_file" ]] && cp "$so_file" "${outdir}/"
+ if [[ ! -f "$so_file" ]]; then
+ echo "::error::expected connector plugin artifact missing:
${so_file}. The plugin crate built no cdylib, or its name does not match
lib<crate>.so." >&2
+ exit 1
Review Comment:
This turns a previously tolerated miss into a hard failure at the same time
as the plugin set silently grows, so the first master push after merge is the
first real exercise of the new set.
Old behaviour was `[[ -f "$so_file" ]] && cp "$so_file" "${outdir}/"`, a
soft skip. Combined with `default: ""` at line 49, the derived list goes from
the 10 hardcoded crates to 17: `clickhouse_sink`, `delta_sink`, `doris_sink`,
`http_sink`, `influxdb_sink`, `influxdb_source`, `mongodb_sink` are new here.
Any of those that does not emit `lib<crate>.so` on `x86_64-unknown-linux-gnu` /
`aarch64-unknown-linux-gnu` now fails the edge release rather than being
skipped.
The hard failure is the right call, the concern is that it is unverified for
the 7 additions. Per the PR description the local validation was the docker
image build, which is a different job and a different build graph.
`timeout-minutes: 60` is also unchanged for what is now a 17-crate release
build on the standard runners, and the tarball grows accordingly (delta/arrow,
iceberg, mongodb, surrealdb closures).
Worth running the `Connector plugins` job on this branch before merge to
confirm all 17 produce a cdylib on both targets and that it fits in the timeout.
--
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]
