nsivabalan commented on code in PR #13276:
URL: https://github.com/apache/hudi/pull/13276#discussion_r2080831783
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -251,7 +260,7 @@ public List<MetadataPartitionType>
getEnabledPartitionTypes() {
protected boolean initializeIfNeeded(HoodieTableMetaClient dataMetaClient,
Option<String>
inflightInstantTimestamp) throws IOException {
HoodieTimer timer = HoodieTimer.start();
- List<MetadataPartitionType> metadataPartitionsToInit = new
ArrayList<>(MetadataPartitionType.getValidValues().length);
+ List<MetadataPartitionType> metadataPartitionsToInit = new
ArrayList<>(MetadataPartitionType.values().length);
Review Comment:
this might return 1 additional value compared to getValidValues().
we ignore ALL_PARTITIONS in getValidValues().
##########
hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java:
##########
@@ -179,6 +180,12 @@ public void constructMetadataPayload(HoodieMetadataPayload
payload, GenericRecor
}
},
EXPRESSION_INDEX(PARTITION_NAME_EXPRESSION_INDEX_PREFIX, "expr-index-", -1) {
+ @Override
+ public boolean isMetadataPartitionSupported(HoodieTableMetaClient
metaClient) {
+ // Partition stats is supported for partitioned tables only
Review Comment:
this comment does not make sense here.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -193,8 +194,16 @@ protected
HoodieBackedTableMetadataWriter(StorageConfiguration<?> storageConf,
ValidationUtils.checkArgument(!initialized || this.metadata != null, "MDT
Reader should have been opened post initialization");
}
+ @VisibleForTesting
List<MetadataPartitionType> getEnabledPartitions(HoodieMetadataConfig
metadataConfig, HoodieTableMetaClient metaClient) {
- return MetadataPartitionType.getEnabledPartitions(metadataConfig,
metaClient);
+ if (!metadataConfig.isEnabled()) {
+ return Collections.emptyList();
+ }
+ return MetadataPartitionType.getValidValues().stream()
+ .filter(partitionType ->
partitionType.isMetadataPartitionSupported(metaClient)
+ && (partitionType.isMetadataPartitionEnabled(metadataConfig)
+ || partitionType.isMetadataPartitionAvailable(metaClient)))
+ .collect(Collectors.toList());
Review Comment:
not sure if we can simplify.
each if meant to serve diff purpose.
a. isMetadataPartitionSupported -> some partitions are not supported in v6,
but only version 8 and above.
b. isMetadataPartitionEnabled -> based on writer properties, we determine
which partitions are enabled.
c. isMetadataPartitionAvailable -> For secondary and expression index, even
if writer properties does not suggest to be enabled, if its already enabled and
available, we wanted to update these partitions. Also, incase of async metadata
partition initialization, even if current writer properties does not have a
certain partition as enabled, if its part of inflight metadata partitions in
table config, we wanted to update these partitions.
So, not sure how we can fold all these into one method.
##########
hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java:
##########
@@ -200,6 +207,12 @@ public String getPartitionPath(HoodieTableMetaClient
metaClient, String indexNam
}
},
SECONDARY_INDEX(HoodieTableMetadataUtil.PARTITION_NAME_SECONDARY_INDEX_PREFIX,
"secondary-index-", 7) {
+ @Override
+ public boolean isMetadataPartitionSupported(HoodieTableMetaClient
metaClient) {
+ // Partition stats is supported for partitioned tables only
Review Comment:
same here
--
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]
