rangareddy commented on code in PR #19491: URL: https://github.com/apache/hudi/pull/19491#discussion_r3932312293
########## 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: Both added, and your arithmetic checks out: I built the bundle the way this step does and hudi-hadoop-common contributes 14 of the 102 classes, so its loss leaves 88 and the old floor of 100 was the only thing standing in the way. `HoodieROTablePathFilter` and `HadoopFSUtils` are both sentinels now. ########## 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: Added for both, matched with a leading space so the relocated `org/apache/hudi/org/apache/avro/**` entries are not counted. Confirmed against a locally built bundle: unrelocated avro 0, codehaus jackson 0, and parquet 5 - exactly the `SchemaRepair` and `ParquetConfiguration` you named, so it correctly stays out of the 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. Review Comment: Went with the sentinels rather than the note: there is now one per include, covering kryo, minlog, objenesis, avro, commons-io, commons-lang3, protobuf and jol-core (relocated) plus caffeine (unrelocated by design). Every path was verified against a real bundle. Two are deliberately uncovered and the script says why: parquet-avro lands under the bootstrap prefix that `Message` already guards, and metrics-core contributes 0 entries, which I confirmed - I have left removing that dead include to #19490 cleanup as you suggest. ########## .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: Kept the rebuild, as the labelled repository-path guard you offer at the end, and the comment now states that explicitly: `Build Project` builds from the full reactor, where `ReactorReader` serves the effective model rather than the published dependency-reduced POM, so its jar cannot cover the resolution path that broke. You are right that `-am` and repository resolution give the same graph today; the check is guarding against that ceasing to be true. ########## .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: Applied your suggestion. Installing `hudi-hadoop-mr -am` and `clean package` on presto is also what I used locally to build the jar I validated the script against, so the closure is confirmed sufficient. Agreed on the two-producers point being the real problem, rather than the content differing today. ########## 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: Both applied: a here-string, so green runs no longer log the broken pipe once per sentinel, and the floor now counts classes (102) rather than classes plus directory entries (110). That second one mattered more than a nit - the inflated count was what let the hudi-hadoop-common case slip under the floor. -- 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]
