voonhous commented on code in PR #19726:
URL: https://github.com/apache/hudi/pull/19726#discussion_r3888879131
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -229,6 +229,15 @@ public void teardown() throws Exception {
HiveTestUtil.clear();
}
+ @Test
+ public void testUpdateLastCommitTimeSyncedWithNoCompletedCommit() {
+ hiveSyncProps.setProperty(HIVE_SYNC_MODE.key(), "hms");
+ // created-but-never-written table: the active timeline has no completed
instant, the update must no-op
+ reInitHiveSyncClient();
+ assertDoesNotThrow(() ->
hiveClient.updateLastCommitTimeSynced(HiveTestUtil.TABLE_NAME));
+ assertFalse(hiveClient.tableExists(HiveTestUtil.TABLE_NAME), "the no-op
must not create or alter the metastore table");
Review Comment:
**nit:** Feel free to ignore. No branch of `updateLastCommitTimeSynced` can
create a table, so this holds before the call too. `assertDoesNotThrow` on the
line above is what discriminates: delete the early return at
`HoodieHiveSyncClient.java:725` and `client.getTable` throws.
Could this line go, or the comment note that line 237 is the real guard so a
later reader does not trim the wrong one?
##########
hudi-aws/src/test/java/org/apache/hudi/aws/sync/TestAWSGlueSyncClient.java:
##########
@@ -901,7 +976,7 @@ private CompletableFuture<GetTableResponse>
getTableWithDefaultProps(String tabl
.parameters(new HashMap<>())
.storageDescriptor(storageDescriptor)
.partitionKeys(partitionColumns)
- .parameters(tableProperties)
+ .parameters(extraTableProperties)
Review Comment:
**nit:** Feel free to ignore. The builder already calls `.parameters(new
HashMap<>())` a few lines above and this overwrites it (SDK v2 builder setters
are last-write-wins), so the earlier call is dead. Pre-existing, but the new
parameter name makes it read as though the two merge.
Could the earlier `.parameters(new HashMap<>())` line be dropped, and this
parameter renamed `tableProperties`?
##########
hudi-aws/src/test/java/org/apache/hudi/aws/sync/TestAWSGlueSyncClient.java:
##########
@@ -881,13 +885,84 @@ void testUpdateHoodieWriterVersionThrowsGlueException()
throws ExecutionExceptio
assertTrue(ex.getMessage().contains(HoodieVersion.get()), "exception
message should mention the writer version");
}
+ @Test
+ void testUpdateLastCommitTimeSyncedPersistsCompletionTimeWatermark() throws
ExecutionException, InterruptedException, IOException {
+ String tableName = "test";
+ List<Column> columns =
Collections.singletonList(GlueTestUtil.getColumn("name", "string", "person's
name"));
+ List<Column> partitionKeys =
Collections.singletonList(GlueTestUtil.getColumn("city", "string", "person's
city"));
+ CompletableFuture<UpdateTableResponse> mockUpdateTableResponse =
mock(CompletableFuture.class);
+
Mockito.when(mockUpdateTableResponse.get()).thenReturn(UpdateTableResponse.builder().build());
+ Mockito.when(mockAwsGlue.getTable(any(GetTableRequest.class)))
+ .thenReturn(getTableWithProps(tableName, columns, partitionKeys,
Collections.singletonMap("EXTERNAL", "TRUE")));
+
Mockito.when(mockAwsGlue.updateTable(any(UpdateTableRequest.class))).thenReturn(mockUpdateTableResponse);
+
+ // A second commit whose instant time sorts below the fixture's but whose
completion time is later:
+ // the instant-time watermark must come from the fixture commit and the
completion-time watermark from this one.
+ String earlierInstant = "100";
+ String earlierInstantCompletionTime = "20250101000001000";
+ GlueTestUtil.createCommitFile(earlierInstant,
earlierInstantCompletionTime);
+ // fresh client so its lazily loaded timeline sees both commits
+ String basePath =
GlueTestUtil.getHiveSyncConfig().getString(META_SYNC_BASE_PATH);
+ awsGlueSyncClient = new AWSGlueCatalogSyncClient(mockAwsGlue, mockSts,
GlueTestUtil.getHiveSyncConfig(),
+
HoodieTableMetaClient.builder().setConf(HadoopFSUtils.getStorageConf(GlueTestUtil.getHadoopConf())).setBasePath(basePath).build());
+
+ awsGlueSyncClient.updateLastCommitTimeSynced(tableName);
+
+ ArgumentCaptor<UpdateTableRequest> captor =
ArgumentCaptor.forClass(UpdateTableRequest.class);
+ verify(mockAwsGlue, times(1)).updateTable(captor.capture());
+ Map<String, String> params = captor.getValue().tableInput().parameters();
+ assertEquals(GlueTestUtil.INSTANT_TIME,
params.get(HOODIE_LAST_COMMIT_TIME_SYNC));
+ assertEquals(earlierInstantCompletionTime,
params.get(HOODIE_LAST_COMMIT_COMPLETION_TIME_SYNC),
+ "incremental sync needs the completion-time watermark to pick up
commits that complete out of instant-time order");
+ assertEquals("TRUE", params.get("EXTERNAL"),
+ "updating the watermarks must merge into the existing table
parameters, not replace them");
+ }
+
+ @Test
+ void testUpdateLastCommitTimeSyncedWithNoCompletedCommit() throws
IOException {
+ String tableName = "test";
+ GlueTestUtil.deleteCommitFile(GlueTestUtil.INSTANT_TIME,
GlueTestUtil.INSTANT_COMPLETION_TIME);
+ String basePath =
GlueTestUtil.getHiveSyncConfig().getString(META_SYNC_BASE_PATH);
+ awsGlueSyncClient = new AWSGlueCatalogSyncClient(mockAwsGlue, mockSts,
GlueTestUtil.getHiveSyncConfig(),
+
HoodieTableMetaClient.builder().setConf(HadoopFSUtils.getStorageConf(GlueTestUtil.getHadoopConf())).setBasePath(basePath).build());
+
+ awsGlueSyncClient.updateLastCommitTimeSynced(tableName);
+
+ verify(mockAwsGlue, never()).updateTable(any(UpdateTableRequest.class));
+ }
+
+ @Test
+ void testGetLastCommitCompletionTimeSyncedWhenAbsent() {
Review Comment:
**nit:** Feel free to ignore. These two 8-line tests cover a single
`getOrDefault`, and `WhenAbsent`/`WhenPresent` differs from the
`testX_Scenario` convention the neighbours use
(`testCreateOrReplaceTable_TableExists`, `testGetAllPartitions_SinglePage`).
Could they fold into one
`testGetLastCommitCompletionTimeSynced_AbsentAndPresent`?
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -941,11 +941,10 @@ public Option<String> getLastCommitTimeSynced(String
tableName) {
@Override
public Option<String> getLastCommitCompletionTimeSynced(String tableName) {
- // Get the last commit completion time from the TBLproperties
try {
return
Option.ofNullable(getInitialTable(tableName).parameters().getOrDefault(HOODIE_LAST_COMMIT_COMPLETION_TIME_SYNC,
null));
} catch (Exception e) {
- throw new HoodieGlueSyncException("Failed to get the last commit
completion time synced from the table " + tableName, e);
+ throw new HoodieGlueSyncException("Fail to get last commit completion
time synced for " + tableId(databaseName, tableName), e);
Review Comment:
**nit:** Feel free to ignore. In a `test(sync)` PR, would it be worth
leaving the message rewording out? The new text diverges from the identical
message still in `HoodieHiveSyncClient.java:606`, so the two clients no longer
read the same way for the same failure.
The `HashMap` to `Map` widening below is worth keeping either way.
--
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]
