hudi-agent commented on code in PR #18441:
URL: https://github.com/apache/hudi/pull/18441#discussion_r3250132435
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/HiveIncrementalPuller.java:
##########
@@ -183,7 +204,7 @@ public void saveDelta() throws IOException {
}
}
- private void executeIncrementalSQL(String tempDbTable, String
tempDbTablePath, Statement stmt)
+ void executeIncrementalSQL(String tempDbTable, String tempDbTablePath,
Statement stmt)
Review 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`).
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/HiveIncrementalPuller.java:
##########
@@ -292,13 +298,12 @@ private String scanForCommitTime(FileSystem fs, String
targetDataPath) throws IO
if (!fs.exists(new Path(targetDataPath)) || !fs.exists(new
Path(targetDataPath + "/.hoodie"))) {
return "0";
}
- HoodieTableMetaClient metadata = HoodieTableMetaClient.builder()
-
.setConf(HadoopFSUtils.getStorageConfWithCopy(fs.getConf())).setBasePath(targetDataPath).build();
+ HoodieTableMetaClient metadata =
HoodieTableMetaClient.builder().setConf(fs.getConf()).setBasePath(targetDataPath).build();
Review 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.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
