hudi-agent commented on code in PR #18572:
URL: https://github.com/apache/hudi/pull/18572#discussion_r3289608358
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1417,11 +1438,58 @@ protected void initMetadataTable(Option<String>
instantTime, HoodieTableMetaClie
// by default do nothing.
}
- // TODO: this method will be removed with restore/rollback changes in MDT
+ // TODO: this method will be removed with restore/rollback changes in MDT.
+ // IMPORTANT: Until then, the boolean parameter is silently ignored —
callers that pass `false`
+ // to suppress MDT bootstrap will find that initTable() still re-bootstraps
MDT when
+ // config.isMetadataTableEnabled()=true. The guard in restoreToInstant
compensates for this by
+ // keying off the parameter value rather than the resulting MDT state.
protected final HoodieTable initTable(WriteOperationType operationType,
Option<String> instantTime, boolean initMetadataTable) {
+ if (!initMetadataTable) {
+ log.warn("initTable called with initMetadataTable=false but the flag is
currently ignored; "
+ + "MDT may still be bootstrapped if
config.isMetadataTableEnabled()=true. "
Review Comment:
🤖 nit: this `log.warn` will fire on every internal call from
`restoreToSavepoint` (which intentionally passes `false`), so it's
expected-noise rather than a warning. Could you drop it to `log.debug`, or
remove it since the long block comment above already documents the gotcha?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -909,16 +896,50 @@ public boolean rollback(final String commitInstantTime,
String rollbackInstantTi
}
/**
- * NOTE : This action requires all writers (ingest and compact) to a table
to be stopped before proceeding. Revert
- * the (inflight/committed) record changes for all commits after the
provided instant time.
+ * Revert the (inflight/committed) record changes for all commits after the
provided instant time.
+ *
+ * <p>NOTE: This action requires all writers (ingest and compact) to a table
to be stopped before proceeding.
+ *
+ * <p>When {@code initialMetadataTableIfNecessary=true} (the normal
direct-call path), this method
+ * applies a fail-fast guard that rejects restores to a point strictly older
than the oldest MDT
+ * compaction, because MDT base files do not exist before that point and the
MDT would be left
+ * inconsistent. In that case, use {@link #restoreToSavepoint(String)}
instead, which will delete
+ * the MDT and allow the restore to proceed safely.
+ *
+ * <p>When {@code initialMetadataTableIfNecessary=false} (called internally
by
+ * {@link #restoreToSavepoint(String)} after MDT deletion), the guard is
skipped. Callers must
+ * not pass {@code false} without having already ensured MDT consistency
themselves.
*
* @param savepointToRestoreTimestamp savepoint instant time to which
restoration is requested
+ * @param initialMetadataTableIfNecessary whether to bootstrap the MDT if it
is missing, and
+ * whether to apply the MDT-consistency guard before restoring
*/
public HoodieRestoreMetadata restoreToInstant(final String
savepointToRestoreTimestamp, boolean initialMetadataTableIfNecessary) throws
HoodieRestoreException {
log.info("Begin restore to instant {}", savepointToRestoreTimestamp);
Timer.Context timerContext = metrics.getRollbackCtx();
try {
HoodieTable<T, I, K, O> table = initTable(WriteOperationType.UNKNOWN,
Option.empty(), initialMetadataTableIfNecessary);
+ // We cannot restore to a point older than the oldest compaction on the
MDT when called
+ // directly (not via restoreToSavepoint). restoreToSavepoint() handles
this case by deleting
+ // the MDT and passing initialMetadataTableIfNecessary=false, so the
guard is skipped there.
+ // Note: initTable() currently ignores the initMetadataTable flag and
may re-bootstrap the MDT,
+ // so we use initialMetadataTableIfNecessary as the signal for whether
the guard should apply.
+ if (initialMetadataTableIfNecessary &&
table.getMetaClient().getTableConfig().isMetadataTableAvailable()) {
+ try {
Review Comment:
🤖 nit: the MDT `HoodieTableMetaClient.builder()...build()` construction is
duplicated here and in `restoreToSavepoint`. Consider having
`getMdtInconsistencyReason` build the mdt meta client itself (it already
encapsulates the rest of the check), so both callsites just pass
`savepointTime` + base path.
<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]
