voonhous commented on code in PR #19239:
URL: https://github.com/apache/hudi/pull/19239#discussion_r3698347802
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java:
##########
@@ -291,6 +295,28 @@ protected void syncHoodieTable(String tableName, boolean
useRealtimeInputFormat,
}
}
+ /**
+ * Whether last commit time synced trails the midpoint of the completed
commit instants.
+ * Advancing it at the midpoint bounds how far it can fall behind, capping
the archived-timeline
+ * scans a stale value forces on every conditional-sync round.
+ */
+ @VisibleForTesting
+ boolean isLastCommitTimeSyncedBehindTimelineMidpoint(String tableName) {
+ Option<String> lastCommitTimeSynced =
syncClient.getLastCommitTimeSynced(tableName);
+ if (!lastCommitTimeSynced.isPresent()) {
+ return false;
+ }
+ // Completed commits only: getCommitsTimeline() excludes non-commit
actions (clean, rollback),
+ // and filterCompletedInstants() excludes inflight instants.
+ List<HoodieInstant> completedCommits =
+
syncClient.getMetaClient().getCommitsTimeline().filterCompletedInstants().getInstants();
Review Comment:
**minor:** this reads a different timeline than the one the decision
affects. `metaClient.getCommitsTimeline()` is table-type-aware -- on COW it
returns `getCommitAndReplaceTimeline()`, dropping DELTA_COMMIT -- while
`updateLastCommitTimeSynced`, `isAlreadySynced`, and the fallback check all go
through `syncClient.getActiveTimeline()` (=
`getCommitsTimeline().filterCompletedInstants()`, always including
DELTA_COMMIT). The first commit of this branch (0423e103f06e) had the
consistent accessor; fb8fa74ff0ae switched it to fit the mock. Please restore
it and stub `syncClient.getActiveTimeline()` in
`TestHiveSyncToolTimelineMidpoint` instead of mocking `HoodieTableMetaClient`
-- it also simplifies the test.
```suggestion
// getActiveTimeline() narrows to completed commit actions, matching the
timeline that
// updateLastCommitTimeSynced and isAlreadySynced read.
List<HoodieInstant> completedCommits =
syncClient.getActiveTimeline().getInstants();
```
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java:
##########
@@ -291,6 +295,28 @@ protected void syncHoodieTable(String tableName, boolean
useRealtimeInputFormat,
}
}
+ /**
+ * Whether last commit time synced trails the midpoint of the completed
commit instants.
+ * Advancing it at the midpoint bounds how far it can fall behind, capping
the archived-timeline
+ * scans a stale value forces on every conditional-sync round.
Review Comment:
The archived-timeline scan described here has been unreachable from
Hive/Glue sync since HUDI-5816 (#8388): `validateAndSyncPartitions` diverts to
`syncAllPartitions` whenever
`getActiveTimeline().isBeforeTimelineStarts(marker)`, and since completed
commit actions are a subset of write actions, that always triggers before
`TimelineUtils.getCommitsTimelineAfter` could reach its archived-merge branch
(`writeTimeline.isBeforeTimelineStarts`). The real per-round cost of a stale
marker is the full metastore + storage partition listing in the fallback.
Please update the commit message and PR description too -- the mechanism
matters for choosing the right trigger (see the comment on the gate above).
```suggestion
* Advancing it at the midpoint bounds how far it can fall behind, capping
the repeated
* full partition listings a stale value forces on every conditional-sync
round.
```
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -2536,20 +2536,24 @@ public void testSyncWithoutDiffs(String syncMode)
throws Exception {
HiveTestUtil.addMORPartitions(0, true, true, true,
ZonedDateTime.now().plusDays(2), commitTime2, commitTime3);
+ // No sync condition is met, but the sync marker trails the midpoint of
the active commits
+ // timeline ([100, 101, 102, 103] with midpoint 102), so it advances to
the last commit.
+ reInitHiveSyncClient();
reSyncHiveTable();
- assertEquals(commitTime1,
hiveClient.getLastCommitTimeSynced(tableName).get());
+ assertEquals(commitTime3,
hiveClient.getLastCommitTimeSynced(tableName).get());
// Let the last commit time synced to be before the start of the active
timeline,
- // to trigger the fallback of listing all partitions. There is no
partition change
- // and the last commit time synced should still be the same.
+ // to trigger the fallback of listing all partitions. There is no
partition change,
+ // and the sync marker again trails the timeline midpoint, so it advances
to the
+ // last commit instead of aging out further.
HiveTestUtil.addMORPartitions(0, true, true, true,
ZonedDateTime.now().plusDays(2), commitTime4, commitTime5);
HiveTestUtil.removeCommitFromActiveTimeline(commitTime0, COMMIT_ACTION);
HiveTestUtil.removeCommitFromActiveTimeline(commitTime1,
DELTA_COMMIT_ACTION);
HiveTestUtil.removeCommitFromActiveTimeline(commitTime2, COMMIT_ACTION);
HiveTestUtil.removeCommitFromActiveTimeline(commitTime3,
DELTA_COMMIT_ACTION);
reInitHiveSyncClient();
reSyncHiveTable();
- assertEquals(commitTime1,
hiveClient.getLastCommitTimeSynced(tableName).get());
+ assertEquals(commitTime5,
hiveClient.getLastCommitTimeSynced(tableName).get());
Review Comment:
**blocker (test coverage):** both updated assertions expect exactly what the
always-update path writes (`updateLastCommitTimeSynced` stores
`activeTimeline.lastInstant().requestedTime()`, i.e.
`commitTime3`/`commitTime5`), so this test no longer discriminates the new
gate. Verified by mutation: with the whole condition replaced by `if (true)`,
with the midpoint index changed to `size() - 1`, or with the helper body
replaced by `return true`, the full `TestHiveSyncTool` suite (274 tests) stays
green. The previous assertions were the HUDI-1932 (#3053) pin that a no-diff
conditional sync does not move the marker; nothing replaces that pin
(`testHiveSyncWithMultiWriter` always has partition changes, so it cannot).
Please split this second block to also pin the negative case: after adding
`commitTime4`/`commitTime5` but before the `removeCommitFromActiveTimeline`
calls, sync and assert the marker stays at `commitTime3` -- it is at the
midpoint of `[100..105]`, so it must not advance:
```java
HiveTestUtil.addMORPartitions(0, true, true, true,
ZonedDateTime.now().plusDays(2), commitTime4, commitTime5);
// Marker 103 is at the midpoint of [100..105]: no sync condition met, must
NOT advance.
reInitHiveSyncClient();
reSyncHiveTable();
assertEquals(commitTime3,
hiveClient.getLastCommitTimeSynced(tableName).get());
// ... existing removeCommitFromActiveTimeline calls and final sync unchanged
```
This passes on the PR branch and fails on all three mutants above.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java:
##########
@@ -276,7 +279,8 @@ protected void syncHoodieTable(String tableName, boolean
useRealtimeInputFormat,
boolean partitionsChanged = validateAndSyncPartitions(tableName,
tableExists);
boolean meetSyncConditions = schemaChanged || propertiesChanged ||
partitionsChanged;
- if (!config.getBoolean(META_SYNC_CONDITIONAL_SYNC) ||
meetSyncConditions) {
+ if (!config.getBoolean(META_SYNC_CONDITIONAL_SYNC) || meetSyncConditions
+ || isLastCommitTimeSyncedBehindTimelineMidpoint(tableName)) {
Review Comment:
**major (correctness):** advancing the marker here permanently disables the
only path that can DROP a metastore partition that was not rewritten. The
incremental path emits drops only for partitions also present in
`writtenPartitionsSince` (the 3-arg `HoodieSyncClient.getPartitionEvents` loops
over `writtenPartitionsOnStorage`, and `syncPartitions` returns early when it
is empty). Cleaner-deleted partitions, out-of-band directory deletes, and
manual catalog edits are repaired only by `syncAllPartitions` (HUDI-5816,
#8388), which fires only when the marker falls before the active timeline
start. Today a conditional-sync table self-heals every round once the marker
ages out (at full-listing cost); with this change it never reconciles again.
This drop path is historically fragile (HUDI-9770, #13794 patched it in Aug
2025).
Also, the midpoint is a heuristic for a condition the code already computes
exactly: `validateAndSyncPartitions` knows when the full-listing fallback fired
(`syncClient.getActiveTimeline().isBeforeTimelineStarts(...)`). The midpoint
rule additionally fires on roughly half of the no-change rounds where the
marker is still well inside the active timeline (catalog writes with no
benefit), and gives no bound at all if the commit rate between syncs outpaces
the archival window.
Suggested fix: advance the marker exactly in the round that performed
`syncAllPartitions` -- the catalog is provably consistent with storage at that
point, the every-round full listing still collapses to once per aging-out
cycle, and catalog writes are strictly rarer than with the midpoint rule.
Alternatively, keep the midpoint but force one `syncAllPartitions` in the
advancing round. Either way, please add a test: remove a partition on storage
cleaner-style with no new writes and assert it eventually disappears from the
metastore.
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncToolTimelineMidpoint.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.hive;
+
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.testutils.MockHoodieTimeline;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.sync.common.HoodieSyncClient;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for {@link
HiveSyncTool#isLastCommitTimeSyncedBehindTimelineMidpoint}, which decides
+ * whether a no-change conditional sync should still advance last commit time
synced. The helper
+ * reads the completed commits timeline, so these mock {@code
getMetaClient().getCommitsTimeline()};
+ * a call to any other timeline would hit an unstubbed mock and fail.
+ */
+class TestHiveSyncToolTimelineMidpoint {
+
+ private static final String TABLE_NAME = "table";
+
+ @Test
+ void midpointIsComputedFromCompletedCommitsOnly() {
+ // Completed commits [100, 102, 104], midpoint 102; the later inflight 106
must not shift it to 104.
+ HoodieSyncClient syncClient = mockSyncClient(new
MockHoodieTimeline(Stream.of("100", "102", "104"), Stream.of("106")));
+ HiveSyncTool tool = toolWith(syncClient);
+
+ // Not synced yet: nothing to advance.
+ stubLastCommitTimeSynced(syncClient, Option.empty());
+ assertFalse(tool.isLastCommitTimeSyncedBehindTimelineMidpoint(TABLE_NAME));
+
+ // Trails the midpoint.
+ stubLastCommitTimeSynced(syncClient, Option.of("101"));
+ assertTrue(tool.isLastCommitTimeSyncedBehindTimelineMidpoint(TABLE_NAME));
+
+ // At the midpoint is not behind it.
+ stubLastCommitTimeSynced(syncClient, Option.of("102"));
+ assertFalse(tool.isLastCommitTimeSyncedBehindTimelineMidpoint(TABLE_NAME));
+
+ // Past 102 but below 104: behind only if the inflight 106 is wrongly
counted.
+ stubLastCommitTimeSynced(syncClient, Option.of("103"));
+ assertFalse(tool.isLastCommitTimeSyncedBehindTimelineMidpoint(TABLE_NAME));
+ }
+
+ @Test
+ void midpointHandlesSmallTimelines() {
Review Comment:
nit, feel free to ignore: this packs the size-1 and size-2 scenarios into
one method with mid-method mock reassignment, so a failure report will not say
which size broke. Two `@Test` methods would match the per-scenario style of the
rest of the class.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java:
##########
@@ -291,6 +295,28 @@ protected void syncHoodieTable(String tableName, boolean
useRealtimeInputFormat,
}
}
+ /**
+ * Whether last commit time synced trails the midpoint of the completed
commit instants.
+ * Advancing it at the midpoint bounds how far it can fall behind, capping
the archived-timeline
+ * scans a stale value forces on every conditional-sync round.
+ */
+ @VisibleForTesting
+ boolean isLastCommitTimeSyncedBehindTimelineMidpoint(String tableName) {
+ Option<String> lastCommitTimeSynced =
syncClient.getLastCommitTimeSynced(tableName);
Review Comment:
nit, optional: `getLastCommitTimeSynced` is now read twice per table per
round (here and in `validateAndSyncPartitions`). Free on the cached HMS/Glue
paths, but HMS JDBC-fallback mode re-queries the metastore each time. Consider
hoisting the read into `syncHoodieTable` and passing it to both.
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncToolTimelineMidpoint.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.hive;
+
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.testutils.MockHoodieTimeline;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.sync.common.HoodieSyncClient;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for {@link
HiveSyncTool#isLastCommitTimeSyncedBehindTimelineMidpoint}, which decides
+ * whether a no-change conditional sync should still advance last commit time
synced. The helper
+ * reads the completed commits timeline, so these mock {@code
getMetaClient().getCommitsTimeline()};
+ * a call to any other timeline would hit an unstubbed mock and fail.
+ */
+class TestHiveSyncToolTimelineMidpoint {
+
+ private static final String TABLE_NAME = "table";
+
+ @Test
+ void midpointIsComputedFromCompletedCommitsOnly() {
+ // Completed commits [100, 102, 104], midpoint 102; the later inflight 106
must not shift it to 104.
Review Comment:
**minor (coverage):** three branches of the helper are untested:
- The "excludes non-commit actions (clean, rollback)" claim is unpinned:
`MockHoodieTimeline` can only build `COMMIT_ACTION` instants, and the
`midpointNarrowsToCommitsTimeline` test added in 937c446f7655 was deleted by
the fb8fa74ff0ae refactor. What survives pins the accessor name only.
- Marker ahead of the whole timeline (rollback/restore shrank it): the
helper returns false, which is right, but no test says so. Add: marker `"106"`
vs completed `[100, 102, 104]` and assert false.
- COW is never exercised (both integration tests use MOR, this class stubs
the accessor), so the table-type branch inside
`metaClient.getCommitsTimeline()` runs in no test.
Cheapest fix: add the marker-ahead case here, and one rollback instant in
the `testSyncWithoutDiffs` flow (`HiveTestUtil.addRollbackInstantToTable`,
already used at `TestHiveSyncTool.java:608`) so the non-commit narrowing is
pinned end to end.
--
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]
