bitflicker64 commented on code in PR #3194:
URL: https://github.com/apache/hugegraph/pull/3194#discussion_r3941731435
##########
hugegraph-server/Dockerfile:
##########
@@ -59,8 +59,10 @@ RUN apt-get -q update \
iproute2 \
vim \
&& apt-get clean \
- && rm -rf /var/lib/apt/lists/* \
- && sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g"
./conf/rest-server.properties
+ && rm -rf /var/lib/apt/lists/*
+
+COPY --from=build /pkg/hugegraph-server/apache-hugegraph-server-*/
/hugegraph-server/
Review Comment:
⚠️ With the apt layer now ahead of this `COPY`, its cache key no longer
includes the application source, so the layer is reused until the
`eclipse-temurin:11-jre-jammy` digest changes. What is lost is the
per-source-change reinstall: rebuilding the same commit already hit cache
before, but any source change used to force a fresh `apt-get install`, and now
nothing short of a base image update does.
This only bites on a registry-cache flow, and that flow is out of tree:
`docker/bake.hcl` gates every `cache-to` behind `EXPORT_CACHE`, which defaults
to `false`, and no workflow here runs `bake` to build or push. The PR's own
numbers show the effect, with eight runtime layers restored in 2.0-9.3 s
"without rerunning package installation".
Could you add a documented way to force a reinstall, for example `ARG
RUNTIME_DEPS_EPOCH=1` immediately before the apt block in all four files,
bumped when packages need refreshing? `--no-cache-filter` is not an option as
things stand, since the runtime stage has no `AS` name. A short note on the
cache behaviour in `docker/README.md` would help whoever operates the publish
flow.
##########
hugegraph-server/Dockerfile:
##########
@@ -28,14 +28,14 @@ ARG MAVEN_ARGS
ARG SOURCE_REVISION=local
RUN
--mount=type=cache,id=hugegraph-maven-${SOURCE_REVISION},target=/root/.m2,sharing=locked
\
- mvn install $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true
-Dmaven.javadoc.skip=true \
+ mvn install -pl
hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist
\
+ -am $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true
-Dmaven.javadoc.skip=true \
Review Comment:
🧹 The module list is hardcoded ahead of `$MAVEN_ARGS`, which is the only
Maven knob `docker/bake.hcl` exposes. Maven accumulates repeated `-pl` values
rather than letting a later one replace an earlier one, so `MAVEN_ARGS` can
still add modules or drop them with a `!` prefix, but it can no longer set the
scope outright. The PR description notes that the fork experiments "enabled the
same module selection through `MAVEN_ARGS`"; that route closes here.
Drift between the four copies is already covered by the `docker-bake-check`
job in `.github/workflows/docker-build-ci.yml`, so this is only about
overridability.
Suggested change: hoist the list into a build arg beside the existing `ARG
MAVEN_ARGS` and pass it through `_common.args` in `docker/bake.hcl`.
```dockerfile
ARG
MAVEN_PROJECTS="hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist"
```
with the command becoming `mvn install -pl "$MAVEN_PROJECTS" -am $MAVEN_ARGS
...`. All four files stay byte-identical, so the CI identity check still passes.
##########
hugegraph-server/Dockerfile:
##########
@@ -28,14 +28,14 @@ ARG MAVEN_ARGS
ARG SOURCE_REVISION=local
RUN
--mount=type=cache,id=hugegraph-maven-${SOURCE_REVISION},target=/root/.m2,sharing=locked
\
- mvn install $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true
-Dmaven.javadoc.skip=true \
+ mvn install -pl
hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist
\
+ -am $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true
-Dmaven.javadoc.skip=true \
&& rm ./hugegraph-server/*.tar.gz ./hugegraph-pd/*.tar.gz
./hugegraph-store/*.tar.gz
Review Comment:
🧹 The reactor is scoped but the build context is not, so the caching win is
smaller than the benchmark suggests. `.dockerignore` at head excludes only
build output, archives, IDE and OS files, `.git`, `.github`, `**/*.md` and the
compose files, so `hugegraph-test`, `hg-pd-test`, `hg-store-test`,
`hugegraph-cluster-test/**`, `hugegraph-example`, `hg-pd-cli`, `hg-store-cli`
and `install-dist` all still land in the context and still feed the `COPY . .`
cache key on line 25. Editing any of them therefore invalidates this shared
build stage and pays for a full scoped Maven run, for modules the build no
longer compiles.
A follow-up rather than a change here, but worth recording. One caveat for
whoever picks it up: these directories cannot be ignored wholesale, because the
root pom lists them in `<modules>` and Maven fails when a listed module's
`pom.xml` is absent, so only their `src/` subtrees can be excluded.
##########
hugegraph-server/Dockerfile-hstore:
##########
@@ -61,8 +57,13 @@ RUN apt-get -q update \
iproute2 \
vim \
&& apt-get clean \
- && rm -rf /var/lib/apt/lists/* \
- && sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g"
./conf/rest-server.properties
+ && rm -rf /var/lib/apt/lists/*
+
+COPY --from=build /pkg/hugegraph-server/apache-hugegraph-server-*/
/hugegraph-server/
+# remove hugegraph.properties and rename hstore.properties.template for
default hstore backend
+RUN cd /hugegraph-server/conf/graphs \
+ && rm hugegraph.properties && mv hstore.properties.template
hugegraph.properties
+RUN sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g"
./conf/rest-server.properties
Review Comment:
🧹 Both of these `RUN`s edit only the tree copied on line 62, so they are
invalidated together and the split just costs an image layer. This is the one
file where that layer is avoidable: the plain server Dockerfile also gained a
standalone `sed` layer, but it has a single edit with nothing to fold into.
```suggestion
RUN cd /hugegraph-server/conf/graphs \
&& rm hugegraph.properties && mv hstore.properties.template
hugegraph.properties \
&& cd /hugegraph-server \
&& sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g"
./conf/rest-server.properties
```
The comment on line 63 then covers only part of what the merged `RUN` does,
so it is worth widening at the same time.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
