yihua commented on code in PR #9546:
URL: https://github.com/apache/hudi/pull/9546#discussion_r1313658812
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1003,12 +1003,8 @@ private boolean shouldExecuteMetadataTableDeletion() {
// Only execute metadata table deletion when all the following conditions
are met
// (1) This is data table
// (2) Metadata table is disabled in HoodieWriteConfig for the writer
- // (3) Check `HoodieTableConfig.TABLE_METADATA_PARTITIONS`. Either the
table config
- // does not exist, or the table config is non-empty indicating that
metadata table
- // partitions are ready to use
return !HoodieTableMetadata.isMetadataTable(metaClient.getBasePath())
- && !config.isMetadataTableEnabled()
- && !metaClient.getTableConfig().getMetadataPartitions().isEmpty();
Review Comment:
Why do we check condition (3) before?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
@@ -1282,13 +1284,14 @@ public static Set<String>
getValidInstantTimestamps(HoodieTableMetaClient dataMe
validInstantTimestamps.addAll(getRollbackedCommits(instant,
datasetTimeline));
});
- // add restore instants from MDT.
+ // add restore and rollback instants from MDT.
metadataMetaClient.getActiveTimeline().getRollbackAndRestoreTimeline().filterCompletedInstants()
- .filter(instant ->
instant.getAction().equals(HoodieTimeline.RESTORE_ACTION))
+ .filter(instant ->
instant.getAction().equals(HoodieTimeline.RESTORE_ACTION) ||
instant.getAction().equals(HoodieTimeline.ROLLBACK_ACTION))
.getInstants().forEach(instant ->
validInstantTimestamps.add(instant.getTimestamp()));
- // SOLO_COMMIT_TIMESTAMP is used during bootstrap so it is a valid
timestamp
- validInstantTimestamps.add(createIndexInitTimestamp(SOLO_COMMIT_TIMESTAMP,
PARTITION_INITIALIZATION_TIME_SUFFIX));
+
metadataMetaClient.getActiveTimeline().getDeltaCommitTimeline().filterCompletedInstants()
+ .filter(instant ->
instant.getTimestamp().startsWith(SOLO_COMMIT_TIMESTAMP))
+ .getInstants().forEach(instant ->
validInstantTimestamps.add(instant.getTimestamp()));
Review Comment:
Could the suffix (from `VALID_PARTITION_INITIALIZATION_TIME_SUFFIXES`) be
added to the completed commit, not `SOLO_COMMIT_TIMESTAMP`, as valid MDT
initialization instant times?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
@@ -153,6 +153,8 @@ static boolean isValidSuffix(String suffix) {
// This suffix and all after that are used for initialization of the various
partitions. The unused suffixes lower than this value
// are reserved for future operations on the MDT.
private static final int PARTITION_INITIALIZATION_TIME_SUFFIX = 10; //
corresponds to "010";
+ // we have max of 4 partitions (FILES, COL_STATS, BLOOM, RLI)
+ private static final List<String>
VALID_PARTITION_INITIALIZATION_TIME_SUFFIXES =
Arrays.asList("010","011","012","013");
Review Comment:
Should this be dynamically generated based on the number of indexes /
partition in the metadata table? For example, a new index can be added and
update to this list may be missed.
--
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]
