voonhous commented on code in PR #19491: URL: https://github.com/apache/hudi/pull/19491#discussion_r3920794663
########## packaging/bundle-validation/validate_presto_bundle.sh: ########## @@ -0,0 +1,90 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Asserts that hudi-presto-bundle contains the classes it exists to provide. +# +# maven-shade-plugin does not fail when an artifactSet include matches nothing, so a bundle can silently +# ship without the classes it is supposed to carry and the build still succeeds. That is exactly what +# happened before HUDI #19433: hudi-presto-bundle went from 109 org/apache/hudi/hadoop/** entries to 0, +# losing HoodieParquetInputFormat, and no job noticed. +# +# The caller must build the bundle WITHOUT -am, so its bundle dependencies resolve from the repository +# rather than from the reactor. With -am, Maven's ReactorReader serves the effective model instead of the +# published dependency-reduced POM, and the resolution path that actually broke is never exercised. +# +# Usage: validate_presto_bundle.sh <path-to-hudi-presto-bundle.jar> + +set -e + +JAR=$1 + +if [ -z "$JAR" ]; then + echo "::error::usage: $0 <path-to-hudi-presto-bundle.jar>" + exit 1 +fi + +if [ ! -f "$JAR" ]; then + echo "::error::presto bundle jar not found: $JAR" + exit 1 +fi + +# Only the main artifact carries the shaded classes. The sources and javadoc jars do not, and a glob picks +# them up ahead of it because "-" sorts before "." - which is how this script first failed in CI, reporting +# a missing class against hudi-presto-bundle-<version>-javadoc.jar. Refuse them rather than mislead. +case "$(basename "$JAR")" in + *-sources.jar|*-javadoc.jar|*-tests.jar) + echo "::error::$(basename "$JAR") is not the main artifact. Pass" + echo "::error::packaging/hudi-presto-bundle/target/hudi-presto-bundle-<version>.jar instead." + exit 1 + ;; +esac + +# The class the bundle exists to provide, and the one lost in the regression this guards against. +REQUIRED_CLASSES=( + "org/apache/hudi/hadoop/HoodieParquetInputFormat.class" + "org/apache/hudi/hadoop/realtime/HoodieParquetRealtimeInputFormat.class" + "org/apache/hudi/common/table/HoodieTableMetaClient.class" Review Comment: **major:** `hudi-hadoop-common` is one of the two includes #19433 broke, but no sentinel names a class from it: only the floor catches its loss, and by 6 entries (88 hadoop-mr classes + 6 dirs = 94 vs 100). Presto's COW/MOR-RO path also goes through `HoodieROTablePathFilter`, not the RT input format (rfc-44.md:43-45). Could we add one sentinel for each? ```suggestion "org/apache/hudi/hadoop/realtime/HoodieParquetRealtimeInputFormat.class" "org/apache/hudi/hadoop/HoodieROTablePathFilter.class" "org/apache/hudi/hadoop/fs/HadoopFSUtils.class" "org/apache/hudi/common/table/HoodieTableMetaClient.class" ``` ########## packaging/bundle-validation/validate_presto_bundle.sh: ########## @@ -0,0 +1,90 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Asserts that hudi-presto-bundle contains the classes it exists to provide. +# +# maven-shade-plugin does not fail when an artifactSet include matches nothing, so a bundle can silently +# ship without the classes it is supposed to carry and the build still succeeds. That is exactly what +# happened before HUDI #19433: hudi-presto-bundle went from 109 org/apache/hudi/hadoop/** entries to 0, +# losing HoodieParquetInputFormat, and no job noticed. +# +# The caller must build the bundle WITHOUT -am, so its bundle dependencies resolve from the repository +# rather than from the reactor. With -am, Maven's ReactorReader serves the effective model instead of the +# published dependency-reduced POM, and the resolution path that actually broke is never exercised. +# +# Usage: validate_presto_bundle.sh <path-to-hudi-presto-bundle.jar> + +set -e + +JAR=$1 + +if [ -z "$JAR" ]; then + echo "::error::usage: $0 <path-to-hudi-presto-bundle.jar>" + exit 1 +fi + +if [ ! -f "$JAR" ]; then + echo "::error::presto bundle jar not found: $JAR" + exit 1 +fi + +# Only the main artifact carries the shaded classes. The sources and javadoc jars do not, and a glob picks +# them up ahead of it because "-" sorts before "." - which is how this script first failed in CI, reporting +# a missing class against hudi-presto-bundle-<version>-javadoc.jar. Refuse them rather than mislead. +case "$(basename "$JAR")" in + *-sources.jar|*-javadoc.jar|*-tests.jar) + echo "::error::$(basename "$JAR") is not the main artifact. Pass" + echo "::error::packaging/hudi-presto-bundle/target/hudi-presto-bundle-<version>.jar instead." + exit 1 + ;; +esac + +# The class the bundle exists to provide, and the one lost in the regression this guards against. Review Comment: **minor:** Not blocking. 7 of the 11 third-party includes are reached only transitively, the #19433 shape, and one already matches nothing: `com.yammer.metrics:metrics-core` contributes 0 entries to the jar (hudi is on dropwizard). History: #6839 (JOL, `NoClassDefFoundError: GraphLayout`), #7188 (Disruptor). Could we add one relocated sentinel per transitive include, e.g. `org/apache/hudi/org/openjdk/jol/info/ClassLayout.class`, or at least note here that only the hudi includes are covered? The dead metrics-core include is #19490's jackson cleanup again, probably its own commit. ########## .github/workflows/bot.yml: ########## @@ -1266,6 +1266,26 @@ jobs: sudo chown -R "$USER:$(id -g -n)" hudi-platform-service/hudi-metaserver/target/generated-sources mvn package -T 2 -D"$SCALA_PROFILE" -D"$FLINK_PROFILE" -DdeployArtifacts=true -DskipTests=true $MVN_ARGS -pl packaging/hudi-flink-bundle -am -Davro.version="$FLINK_AVRO_VERSION" -Dparquet.version="$FLINK_PARQUET_VERSION" fi + - name: Validate Presto Bundle Contents + if: needs.changes.outputs.relevant == 'true' + env: + SPARK_PROFILE: ${{ matrix.sparkProfile }} + SCALA_PROFILE: ${{ matrix.scalaProfile }} + run: | + # hudi-presto-bundle shades classes it reaches through other modules, and shade does not fail when + # an artifactSet include matches nothing - see HUDI #19433, where the bundle silently shipped 0 + # instead of 109 org/apache/hudi/hadoop/** entries. Build it WITHOUT -am so its bundle + # dependencies resolve from the repository: with -am the ReactorReader serves the effective model + # rather than the published dependency-reduced POM, and the path that broke is never exercised. Review Comment: **minor:** Stale on master: since #19490 (26c0c084bcff) there is no bundle dependency, and none of hudi-common/hudi-hadoop-common/hudi-hadoop-mr publishes a reduced POM, so `-am` and repository resolution give the same graph. On the only remaining leg (scala-2.12, #19655) `Build Project` has already produced the full-reactor jar, the variant releases ship (`deploy_staging_jars.sh:70`), and this rebuild re-triggers the avro/parquet recompile cascade (~2 min). Could we validate the `Build Project` jar and keep this rebuild, if at all, as a labelled repository-path guard? (The commented-out scala-2.13 branch would need presto in its `-pl` list.) ########## packaging/bundle-validation/validate_presto_bundle.sh: ########## @@ -0,0 +1,90 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Asserts that hudi-presto-bundle contains the classes it exists to provide. +# +# maven-shade-plugin does not fail when an artifactSet include matches nothing, so a bundle can silently +# ship without the classes it is supposed to carry and the build still succeeds. That is exactly what +# happened before HUDI #19433: hudi-presto-bundle went from 109 org/apache/hudi/hadoop/** entries to 0, +# losing HoodieParquetInputFormat, and no job noticed. +# +# The caller must build the bundle WITHOUT -am, so its bundle dependencies resolve from the repository +# rather than from the reactor. With -am, Maven's ReactorReader serves the effective model instead of the +# published dependency-reduced POM, and the resolution path that actually broke is never exercised. +# +# Usage: validate_presto_bundle.sh <path-to-hudi-presto-bundle.jar> + +set -e + +JAR=$1 + +if [ -z "$JAR" ]; then + echo "::error::usage: $0 <path-to-hudi-presto-bundle.jar>" + exit 1 +fi + +if [ ! -f "$JAR" ]; then + echo "::error::presto bundle jar not found: $JAR" + exit 1 +fi + +# Only the main artifact carries the shaded classes. The sources and javadoc jars do not, and a glob picks +# them up ahead of it because "-" sorts before "." - which is how this script first failed in CI, reporting +# a missing class against hudi-presto-bundle-<version>-javadoc.jar. Refuse them rather than mislead. +case "$(basename "$JAR")" in + *-sources.jar|*-javadoc.jar|*-tests.jar) + echo "::error::$(basename "$JAR") is not the main artifact. Pass" + echo "::error::packaging/hudi-presto-bundle/target/hudi-presto-bundle-<version>.jar instead." + exit 1 + ;; +esac + +# The class the bundle exists to provide, and the one lost in the regression this guards against. +REQUIRED_CLASSES=( + "org/apache/hudi/hadoop/HoodieParquetInputFormat.class" + "org/apache/hudi/hadoop/realtime/HoodieParquetRealtimeInputFormat.class" + "org/apache/hudi/common/table/HoodieTableMetaClient.class" +) + +# The bundle shades all of hudi-hadoop-mr and hudi-hadoop-common; it carried 109 such entries when this +# check was written. A floor rather than an exact count, so ordinary additions do not fail the build while +# a collapse to zero still does. +MIN_HADOOP_ENTRIES=100 + +echo "::warning::validate_presto_bundle.sh validating $(basename "$JAR")" + +listing=$(unzip -l "$JAR") + +for class in "${REQUIRED_CLASSES[@]}"; do + if ! echo "$listing" | grep -q " $class$"; then + echo "::error::$class is missing from $(basename "$JAR"). An artifactSet include probably matched no" + echo "::error::artifact. Check that the bundle declares the modules it shades, and that it was built" + echo "::error::without -am so bundle dependencies resolve from the repository." + exit 1 + fi + echo " found $class" +done + +hadoop_entries=$(echo "$listing" | grep -c "org/apache/hudi/hadoop/" || true) +if [ "$hadoop_entries" -lt "$MIN_HADOOP_ENTRIES" ]; then + echo "::error::$(basename "$JAR") has only $hadoop_entries org/apache/hudi/hadoop/** entries," + echo "::error::expected at least $MIN_HADOOP_ENTRIES. The bundle has shrunk; see HUDI #19433." + exit 1 +fi +echo " $hadoop_entries org/apache/hudi/hadoop/** entries (floor $MIN_HADOOP_ENTRIES)" Review Comment: **major:** The script asserts presence only. The repository-resolution build this step produces is the exact variant that carried 623 unrelocated `org/codehaus/jackson/**` classes before #19490 (26c0c084bcff), and #19814 was the same leak shape (`org/apache/avro/**`) on the datahub bundle. Could we add zero-count checks for `org/apache/avro/` and `org/codehaus/jackson/`? Both are 0 on a master build. `org/apache/parquet/` cannot join them: hudi-hadoop-common ships `SchemaRepair` and `ParquetConfiguration` in that package. ########## .github/workflows/bot.yml: ########## @@ -1266,6 +1266,26 @@ jobs: sudo chown -R "$USER:$(id -g -n)" hudi-platform-service/hudi-metaserver/target/generated-sources mvn package -T 2 -D"$SCALA_PROFILE" -D"$FLINK_PROFILE" -DdeployArtifacts=true -DskipTests=true $MVN_ARGS -pl packaging/hudi-flink-bundle -am -Davro.version="$FLINK_AVRO_VERSION" -Dparquet.version="$FLINK_PARQUET_VERSION" fi + - name: Validate Presto Bundle Contents + if: needs.changes.outputs.relevant == 'true' + env: + SPARK_PROFILE: ${{ matrix.sparkProfile }} + SCALA_PROFILE: ${{ matrix.scalaProfile }} + run: | + # hudi-presto-bundle shades classes it reaches through other modules, and shade does not fail when + # an artifactSet include matches nothing - see HUDI #19433, where the bundle silently shipped 0 + # instead of 109 org/apache/hudi/hadoop/** entries. Build it WITHOUT -am so its bundle + # dependencies resolve from the repository: with -am the ReactorReader serves the effective model + # rather than the published dependency-reduced POM, and the path that broke is never exercised. + mvn install -T 2 -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -DskipTests=true $MVN_ARGS \ + -pl packaging/hudi-hadoop-mr-bundle -am + mvn package -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -DskipTests=true $MVN_ARGS \ + -pl packaging/hudi-presto-bundle Review Comment: **minor:** If the rebuild stays: `install -pl packaging/hudi-hadoop-mr-bundle -am` re-shades `packaging/hudi-hadoop-mr-bundle/target/*.jar` under a second command line (no `-D$FLINK_PROFILE`, no `-DdeployArtifacts`), and that is the jar `ci_run.sh` copies for the two IT steps below; same content today, but two producers for the jar under test. And `package` without `clean` only rebuilds the reactor-shaded jar because remote-resources rewrites META-INF each run; maven-jar-plugin's `forceCreation=false` would otherwise skip it. Could we install `hudi-hadoop-mr -am` (same closure) and `clean package` presto? ```suggestion mvn install -T 2 -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -DskipTests=true $MVN_ARGS \ -pl hudi-hadoop-mr -am mvn clean package -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -DskipTests=true $MVN_ARGS \ -pl packaging/hudi-presto-bundle ``` ########## packaging/bundle-validation/validate_presto_bundle.sh: ########## @@ -0,0 +1,90 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Asserts that hudi-presto-bundle contains the classes it exists to provide. +# +# maven-shade-plugin does not fail when an artifactSet include matches nothing, so a bundle can silently +# ship without the classes it is supposed to carry and the build still succeeds. That is exactly what +# happened before HUDI #19433: hudi-presto-bundle went from 109 org/apache/hudi/hadoop/** entries to 0, +# losing HoodieParquetInputFormat, and no job noticed. +# +# The caller must build the bundle WITHOUT -am, so its bundle dependencies resolve from the repository +# rather than from the reactor. With -am, Maven's ReactorReader serves the effective model instead of the +# published dependency-reduced POM, and the resolution path that actually broke is never exercised. +# +# Usage: validate_presto_bundle.sh <path-to-hudi-presto-bundle.jar> + +set -e + +JAR=$1 + +if [ -z "$JAR" ]; then + echo "::error::usage: $0 <path-to-hudi-presto-bundle.jar>" + exit 1 +fi + +if [ ! -f "$JAR" ]; then + echo "::error::presto bundle jar not found: $JAR" + exit 1 +fi + +# Only the main artifact carries the shaded classes. The sources and javadoc jars do not, and a glob picks +# them up ahead of it because "-" sorts before "." - which is how this script first failed in CI, reporting +# a missing class against hudi-presto-bundle-<version>-javadoc.jar. Refuse them rather than mislead. +case "$(basename "$JAR")" in + *-sources.jar|*-javadoc.jar|*-tests.jar) + echo "::error::$(basename "$JAR") is not the main artifact. Pass" + echo "::error::packaging/hudi-presto-bundle/target/hudi-presto-bundle-<version>.jar instead." + exit 1 + ;; +esac + +# The class the bundle exists to provide, and the one lost in the regression this guards against. +REQUIRED_CLASSES=( + "org/apache/hudi/hadoop/HoodieParquetInputFormat.class" + "org/apache/hudi/hadoop/realtime/HoodieParquetRealtimeInputFormat.class" + "org/apache/hudi/common/table/HoodieTableMetaClient.class" +) + +# The bundle shades all of hudi-hadoop-mr and hudi-hadoop-common; it carried 109 such entries when this +# check was written. A floor rather than an exact count, so ordinary additions do not fail the build while +# a collapse to zero still does. +MIN_HADOOP_ENTRIES=100 + +echo "::warning::validate_presto_bundle.sh validating $(basename "$JAR")" + +listing=$(unzip -l "$JAR") + +for class in "${REQUIRED_CLASSES[@]}"; do + if ! echo "$listing" | grep -q " $class$"; then Review Comment: **nit:** Feel free to ignore. `echo "$listing" | grep -q` closes the pipe on the first match, so every green run logs `line 73: echo: write error: Broken pipe` three times (job 96332239856). A here-string avoids it; and at line 82 `grep -c "org/apache/hudi/hadoop/.*\.class$"` would put the floor on classes (102) rather than classes plus directory entries (110). ```suggestion if ! grep -q " $class$" <<< "$listing"; then ``` -- 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]
