fix(flink): Fix reflection ctor signature for AwsGlueCatalogSyncTool in HiveSyncContext#18697
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for the fix! This correctly aligns the reflection call in HiveSyncContext#hiveSyncTool() with the current 3-arg AwsGlueCatalogSyncTool constructor (Properties, Configuration, Option<HoodieTableMetaClient>), resolving the runtime NoSuchMethodException for Flink GLUE sync. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
| new Class<?>[] {Properties.class, org.apache.hadoop.conf.Configuration.class}, | ||
| props, hiveConf)); | ||
| new Class<?>[] {Properties.class, org.apache.hadoop.conf.Configuration.class, Option.class}, | ||
| props, hiveConf, Option.<HoodieTableMetaClient>empty())); |
There was a problem hiding this comment.
do you think we can write a simple UT of HiveSyncContext to validate the instantiation of the AWS_GLUE_CATALOG_SYNC_TOOL_CLASS ?
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for the fix! This aligns the reflection-based instantiation of AwsGlueCatalogSyncTool in HiveSyncContext with its current 3-arg constructor and adds two complementary tests — one in hudi-aws validating end-to-end instantiation through the exact reflection signature, and one in hudi-flink pinning the signature via hasConstructor to catch future drift. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small simplification opportunity in HiveSyncContext.java — the HoodieTableMetaClient type witness on Option.empty() is unnecessary and brings in an otherwise-unused import.
cc @yihua
| new Class<?>[] {Properties.class, org.apache.hadoop.conf.Configuration.class}, | ||
| props, hiveConf)); | ||
| new Class<?>[] {Properties.class, org.apache.hadoop.conf.Configuration.class, Option.class}, | ||
| props, hiveConf, Option.<HoodieTableMetaClient>empty())); |
There was a problem hiding this comment.
🤖 nit: the <HoodieTableMetaClient> type witness isn't needed here — ReflectionUtils.loadClass takes Object... args, so Option.empty() works fine and would also let you drop the HoodieTableMetaClient import that was added solely for this.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
github-actions
Bot
added
size:S
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18697 +/- ##
============================================
- Coverage 68.07% 67.89% -0.18%
- Complexity 28943 28957 +14
============================================
Files 2519 2521 +2
Lines 140664 141040 +376
Branches 17428 17480 +52
============================================
+ Hits 95757 95766 +9
- Misses 37043 37412 +369
+ Partials 7864 7862 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
5 participants
Describe the issue this Pull Request addresses
Closes #18673.
Flink GLUE sync fails at runtime with
NoSuchMethodExceptionbecauseHiveSyncContext#hiveSyncTool()loadsAwsGlueCatalogSyncToolviareflection using a 2-arg signature
(Properties, Configuration), whilethe actual constructor is now 3-arg
(Properties, Configuration, Option<HoodieTableMetaClient>).Summary and Changelog
HiveSyncContext#hiveSyncTool()with thecurrent
AwsGlueCatalogSyncToolconstructor: addOption.classtothe parameter types and pass
Option.<HoodieTableMetaClient>empty().Impact
No public API or user-facing change.
Risk Level
low
Documentation Update
none
Contributor's checklist