voonhous commented on PR #19433: URL: https://github.com/apache/hudi/pull/19433#issuecomment-5162862915
Update after chasing the consistency question properly. Short version: **this is done as scoped, and I am withdrawing the description asks rather than holding it up for them.** **State.** 75/75 checks green on `ed3ccf5`, `mergeable`, no conflicts, both blocking items verified fixed against a clean build. All eight review threads are now resolved. The only thing outstanding is an approving review. **Withdrawing the two Impact asks.** Both were record quality rather than correctness, and the content now lives in the threads instead, which is good enough: - the graph-flattening consequence of promotion is on the `hudi-aws-bundle` thread - the `provided` drop on `hudi-datahub-sync-bundle` is on that thread -- along with a correction to my own earlier comment there, since I had asked you to fix reasoning that only ever existed in the thread and never in the description. Sorry for the noise on that one. One small thing I would still take if you are touching the description anyway, but not worth a round trip on its own: the **"Open question: consistency with the nine already-reduced bundles"** section reads as undecided, and it is decided -- promotion stays on these seven. Retitling it would stop the merged record saying the question was open at merge time. Entirely your call. **Release note** for the seven bundles still stands, at release time rather than in this PR. **Follow-ups to file, revised.** The consistency one changed shape once I looked at what the repo actually does -- details and evidence on the `hudi-aws-bundle` thread: - [ ] **Hoist the shade POM flags to the root pom**, rather than "add promotion to the other nine". The root already carries `maven-shade-plugin` with a shared `<configuration>` that all bundles inherit, so `createDependencyReducedPom`, `promoteTransitiveDependencies` and `dependencyReducedPomLocation` belong there once, per-bundle copies deleted, with a single documented override in `hudi-io`. Two things that reframe this: `hudi-io` sets `createDependencyReducedPom=false` correctly, because it is the only module using `shadedArtifactAttached=true`; and the "already on a Spark/Flink classpath" argument only covers four of the nine, so `hudi-cli-bundle`, `hudi-kafka-connect-bundle`, `hudi-integ-test-bundle`, `hudi-metaserver-server-bundle` and `hudi-timeline-server-bundle` are the five with real exposure and should be verified first. - [ ] presto coverage in `packaging/bundle-validation`, stating in the issue that every `-pl packaging/...` in `.github/workflows` and `scripts/` passes `-am`, so the green Azure run here could not have caught the regression this PR fixes. - [ ] dead relocation entry `org.apache.httpcomponents.` -> `org.apache.hudi.aws.org.apache.httpcomponents.` in `hudi-aws-bundle`. - [ ] `hudi-presto-bundle` declaring `hudi-hadoop-mr-bundle`, a fat jar duplicating classes it already shades. Happy for these to be filed by either of us -- say which you would rather pick up. -- 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]
