fix: Fix Scanner resource leak in HiveIncrementalPuller#18441
Conversation
linliu-code
force-pushed
the
fix_hive_leak
branch
2 times, most recently
from
877eaad to
2e04da9
Compare
linliu-code
changed the title
linliu-code
force-pushed
the
fix_hive_leak
branch
2 times, most recently
from
85f5935 to
53ad69a
Compare
yihua
left a comment
yihua
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 contributing! The resource leak fix and early validation are solid improvements. Just one design concern: the SQL file is now read twice (once for validation, once for execution) — an inline comment has details on how to consolidate that.
| } | ||
| } | ||
|
|
||
| private void validateIncrementalSQL() throws IOException { |
There was a problem hiding this comment.
🤖 The SQL file is now read twice — once here for validation and again in executeIncrementalSQL. Could this method return the validated SQL string so it can be passed into executeIncrementalSQL instead of re-reading? That would also eliminate a subtle TOCTOU window where the file could change between the two reads.
| } | ||
|
|
||
| private void executeIncrementalSQL(String tempDbTable, String tempDbTablePath, Statement stmt) | ||
| void executeIncrementalSQL(String tempDbTable, String tempDbTablePath, Statement stmt) |
There was a problem hiding this comment.
🤖 Now that validation was moved to validateIncrementalSQL, this method still declares throws FileNotFoundException and re-reads the file. If the validated SQL were passed in as a parameter, this method wouldn't need file access at all — making it easier to test without a real file on disk.
…mentalPuller - Extract validateIncrementalSQL() and call it at the top of saveDelta(), before any JDBC connection is opened, so a misconfigured SQL file fails fast without issuing a DROP TABLE or consuming a connection slot. - Wrap Scanner in try-with-resources to close the file handle after reading. - Widen executeIncrementalSQL visibility to package-private to allow unit testing the SQL template rendering with a mocked Statement. - Add TestHiveIncrementalPullerExecuteSQL covering: correct SQL rendering, file-not-found, and JDBC execution failure cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 contribution! The Scanner try-with-resources and validation-before-JDBC refactor look good and the new unit tests are a nice addition. However, the diff also includes some unrelated changes to scanForCommitTime and getLastCommitTimePulled that appear to break compilation against current master — please take a look at the inline comments. This should be ready for a Hudi committer or PMC member to take it from here once those are sorted. One naming/visibility nit below.
| } | ||
| HoodieTableMetaClient metadata = HoodieTableMetaClient.builder() | ||
| .setConf(HadoopFSUtils.getStorageConfWithCopy(fs.getConf())).setBasePath(targetDataPath).build(); | ||
| HoodieTableMetaClient metadata = HoodieTableMetaClient.builder().setConf(fs.getConf()).setBasePath(targetDataPath).build(); |
There was a problem hiding this comment.
🤖 These two hunks look like they'd break compilation against current master: HoodieTableMetaClient.Builder#setConf only accepts StorageConfiguration<?> (not the Configuration returned by fs.getConf()), and HoodieInstant has requestedTime() but no getTimestamp() method. The same applies to the HoodieInstant::getTimestamp method reference in getLastCommitTimePulled and the removal of the HadoopFSUtils import. Was this PR rebased onto an older base, or is this an intentional revert? Either way it looks unrelated to the Scanner-leak fix described in the PR and should probably be dropped from this change.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| } | ||
|
|
||
| private void executeIncrementalSQL(String tempDbTable, String tempDbTablePath, Statement stmt) | ||
| void executeIncrementalSQL(String tempDbTable, String tempDbTablePath, Statement stmt) |
There was a problem hiding this comment.
🤖 nit: could you add @VisibleForTesting here? The method was private and is now package-private purely to allow the new test to call it directly, but that intent isn't obvious to a future reader. The rest of the utilities module uses this Guava annotation consistently for the same pattern (e.g. HoodieMetadataTableValidator).
- AI-generated; verify before applying. React 👍/👎 to flag quality.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #18441 +/- ##
=============================================
- Coverage 68.14% 54.00% -14.14%
+ Complexity 29094 12459 -16635
=============================================
Files 2517 1434 -1083
Lines 141113 72161 -68952
Branches 17508 8245 -9263
=============================================
- Hits 96160 38974 -57186
+ Misses 37046 29691 -7355
+ Partials 7907 3496 -4411
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
#18440
HiveIncrementalPuller has two related bugs in executeIncrementalSQL:
Summary and Changelog
Summary: SQL file validation now happens eagerly before any JDBC work, and the file handle is properly released after reading.
Impact
No public API or user-facing behavior change. The only observable difference is that a misconfigured incrementalSQLFile now throws before a JDBC connection is opened rather than after.
Risk Level
Low. The logic is unchanged — validation conditions are identical to before. The refactor only reorders when validation occurs (earlier) and ensures the Scanner is closed. Covered by new unit tests.
Documentation Update
Contributor's checklist