yihua commented on code in PR #19717:
URL: https://github.com/apache/hudi/pull/19717#discussion_r4018993202
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -435,8 +453,16 @@ private void initializeMetadataPartition(
Indexer indexer,
String dataTableInstantTime,
Map<String, List<FileInfo>> partitionToAllFilesMap,
- Lazy<List<FileSliceAndPartition>> lazyMergedFileSlices) throws
IOException {
- String instantTimeForPartition =
generateUniqueInstantTime(dataTableInstantTime);
+ Lazy<List<FileSliceAndPartition>> lazyMergedFileSlices,
+ Option<String> requestedIndexPartitionOpt) throws IOException {
+ // A requested partition initializes under a fresh solo-family instant,
never the indexing
+ // action's own instant. The action's completion applies its data commit
to the metadata table
+ // too, and finding that instant already completed there is treated as a
partially applied
+ // earlier commit: it is rolled back and re-applied, destroying the
initialization records
+ // while leaving the file groups. The solo family is the established shape
for
+ // metadata-table-only bootstrap commits and survives that reconciliation.
+ String instantTimeForPartition = requestedIndexPartitionOpt.isPresent()
+ ? generateUniqueSoloInstantTime() :
generateUniqueInstantTime(dataTableInstantTime);
Review Comment:
Moving the bootstrap onto a solo instant takes it out of the HUDI-5733
exemption, and I think that lets a concurrent writer roll it back
mid-bootstrap. BaseHoodieTableServiceClient.getInstantsToRollback keeps an
inflight MDT deltacommit for rollback unless
isIndexingCommit(dataIndexTimeline, entry) is true, and a solo instant is not
on the data table's INDEXING_ACTION timeline, so the filter that exists
specifically to protect the async indexer no longer matches.
Interleaving, all defaults:
- t0: indexing instant I=20260915120000000 goes inflight;
buildMetadataPartitions runs with no lock held (the only beginStateChange is in
updateTableConfigAndTimeline).
- t1: initializeMetadataPartition picks S=00000000000000002,
initializeFileGroups, then bulkCommit(S) opens S.deltacommit.inflight. A
whole-table secondary-index bootstrap keeps this inflight for minutes or hours.
- t2: an ingest writer commits data instant D=20260915120500000. Its MDT
write client is EAGER, since HoodieTable.getMetadataWriter passes EAGER and
createMetadataWriteConfig only downgrades to LAZY under MDT multi-writer or
streaming writes, both off by default.
- t3: that writer's startCommit calls
CleanerUtils.rollbackFailedWrites(EAGER, COMMIT_ACTION, ...), which reaches
getInstantsToRollback(mdt, EAGER, Option.empty()). S is inflight and not an
indexing commit, so it is rolled back under the running bootstrap.
Depending on where it lands the indexer then either dies on the "FileGroup
count for MDT partition ... should be > 0" check or completes S over a
partially deleted file set, leaving the index marked complete with missing
records. Pre-PR this interleaving was benign because the commit sat at I. Would
it work to extend the exemption to pending solo-family instants, or to hold the
txn lock across buildMetadataPartitions?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/BaseIndexer.java:
##########
@@ -51,6 +56,59 @@ protected BaseIndexer(
this.dataTableMetaClient = dataTableMetaClient;
}
+ /**
+ * Resolves which partition of a definition-driven index type to initialize.
+ * <p>
+ * An indexing action names the partition in the context, and that partition
is initialized
+ * whenever its index definition exists, regardless of how many other
definitions of the type
+ * are still uninitialized. A regular write names nothing, and the partition
is inferred from
+ * the uninitialized definitions: exactly one means that one, any other
count means nothing
+ * is initialized. A requested partition without a definition goes through
the same inference
+ * (that is where a first-time index mints its definition from the write
config), but the
+ * inference only answers for the partition that was asked for: when it
resolves to a single
+ * partition that is a different index, or cannot resolve to exactly one at
all, the action
+ * fails rather than building another index while the requested partition is
marked complete.
+ *
+ * @param context the initialization context
+ * @param uninitializedPartitions the uninitialized partitions of this
type, as the definition
+ * lookup reports them
+ * @param indexType the index type, for the messages
+ * @return the partitions to initialize: exactly one, or none
+ */
+ protected Set<String> resolvePartitionsToInit(IndexInitializationContext
context,
+ Set<String>
uninitializedPartitions,
+ MetadataPartitionType
indexType) {
+ Option<String> requested = context.requestedIndexPartition();
+ if (requested.isPresent()
+ &&
dataTableMetaClient.getTableConfig().getMetadataPartitions().contains(requested.get()))
{
+ // Already initialized, typically by the write that committed between
scheduling and running the
+ // indexing action. Re-initializing would commit at an instant the
metadata table already holds
+ // completed, and the rollback-and-recommit inside that commit destroys
the earlier commit's records.
+ log.info("Metadata partition {} is already initialized, skipping",
requested.get());
+ return Collections.emptySet();
+ }
+ if (requested.isPresent() &&
dataTableMetaClient.getIndexForMetadataPartition(requested.get()).isPresent()) {
+ return Collections.singleton(requested.get());
+ }
+ // A single candidate answers for a request only when it is the requested
partition itself, which is what a
+ // first-time index looks like once the lookup has minted its definition.
Any other single candidate is a
+ // different index, and building it would leave the requested partition
marked complete with nothing in it.
+ if (uninitializedPartitions.size() == 1
+ && (!requested.isPresent() ||
uninitializedPartitions.contains(requested.get()))) {
+ return uninitializedPartitions;
+ }
+ if (requested.isPresent()) {
+ throw new HoodieMetadataException(String.format(
Review Comment:
This throw is a HoodieMetadataException, and RunIndexActionExecutor only
catches IOException, so abort() never runs: the requested partition is left in
hoodie.table.metadata.partitions.inflight and the indexing instant stays
inflight. Re-running execute fails validateAndGetIndexInstant because the
instant is no longer REQUESTED, and re-scheduling returns empty because
ScheduleIndexActionExecutor subtracts inflight-or-completed partitions, so a
single mistyped index name wedges that name until the table config is
hand-edited. Failing is still much better than the old behavior here, it just
needs to route through abort().
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/secondary/SecondaryIndexer.java:
##########
@@ -73,11 +73,8 @@ public SecondaryIndexer(
@Override
public List<IndexInitializationPlan>
buildInitialization(IndexInitializationContext context) throws IOException {
- Set<String> secondaryIndexPartitionsToInit =
getSecondaryIndexPartitionsToInit(SECONDARY_INDEX,
dataTableWriteConfig.getMetadataConfig(), dataTableMetaClient);
- if (secondaryIndexPartitionsToInit.size() > 1) {
- log.warn("Skipping secondary index initialization as only one secondary
index bootstrap at a time is supported for now. Provided: {}",
secondaryIndexPartitionsToInit);
- return Collections.emptyList();
- }
+ Set<String> secondaryIndexPartitionsToInit =
resolvePartitionsToInit(context,
Review Comment:
The lookup is the argument to resolvePartitionsToInit, so it runs before the
already-initialized skip can decide to do nothing, and getIndexPartitionsToInit
persists a new index definition when the uninitialized set comes back empty. An
action for a partition that completed between scheduling and running therefore
builds nothing but still writes a definition for whatever the run config names,
and that dangling uninitialized definition is exactly the precondition for the
wrong-index case on the next action. Could the skip check move ahead of the
lookup?
--
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]
