hudi-agent commented on code in PR #18182:
URL: https://github.com/apache/hudi/pull/18182#discussion_r3336219952
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
@@ -2294,6 +2311,98 @@ public static String
deleteMetadataTablePartition(HoodieTableMetaClient dataMeta
return null;
}
+ /**
+ * Returns true if bucketing is enabled for the metadata table.
+ *
+ * <p>Reads the property from the MDT's own table config so we avoid an
extra disk read of the
+ * data table's hoodie.properties on every file-slice lookup. The property
is persisted on both
+ * the data table and the MDT during MDT initialization (see
+ * {@link #setMetadataTablePartitionBucketing(HoodieTableMetaClient,
HoodieTableMetaClient, boolean)}).
+ *
+ * @param metaClient MetaClient for the metadata table
+ * @return true if bucketing is enabled
+ */
+ private static boolean isBucketingEnabledForMDT(HoodieTableMetaClient
metaClient) {
+ return
metaClient.getTableConfig().isMetadataTablePartitionBucketingEnabled();
+ }
+
+ /**
+ * Returns the list of partitions to list for file slices.
+ * When bucketing is enabled, returns the bucket sub-directories.
+ * When bucketing is disabled, returns the partition itself.
+ *
+ * <p>Note: bucketing is fixed at MDT initialization time (see
+ * {@link
org.apache.hudi.metadata.HoodieBackedTableMetadataWriter#initializeFileGroups}).
A given
+ * MDT is therefore either fully bucketed or fully non-bucketed; mixed-state
file groups under
+ * partition/ alongside partition/<bucket>/ cannot occur. We
deliberately do not also scan the
+ * partition root when bucketing is enabled.
+ *
+ * @param metaClient MetaClient for the metadata table
+ * @param partition The partition path
+ * @param bucketingEnabled Whether bucketing is enabled
+ * @return List of partition paths to list
+ */
+ private static List<String> getPartitionsToList(HoodieTableMetaClient
metaClient, String partition, boolean bucketingEnabled) {
+ List<String> partitionsToList = new ArrayList<>();
+ try {
+ if (bucketingEnabled) {
+ // Find all the buckets in the partition
+ StoragePath partitionPath = new StoragePath(metaClient.getBasePath(),
partition);
+ List<StoragePathInfo> statuses =
metaClient.getStorage().listDirectEntries(partitionPath);
+ for (StoragePathInfo status : statuses) {
+ if (status.isDirectory()) {
+ partitionsToList.add(partition + StoragePath.SEPARATOR +
status.getPath().getName());
+ }
+ }
+ log.info("Listing {} buckets in metadata table partition {}",
partitionsToList.size(), partition);
+ } else {
+ // Only list the partition folder as bucketing is not enabled
+ partitionsToList.add(partition);
+ log.info("Listing non-bucketed metadata table partition {}",
partition);
+ }
+ } catch (IOException e) {
+ throw new HoodieMetadataException("Failed to check for bucketing in
metadata table partition " + partition, e);
+ }
+ return partitionsToList;
+ }
+
+ /**
+ * Set the bucketing of partitions within the metadata table.
+ *
+ * <p>The property is persisted on both the data table (authoritative source
for MDT initialization
+ * decisions) and the MDT (to allow reader code to determine bucketing
without loading data table
+ * properties on every call).
+ *
+ * @param dataMetaClient MetaClient for the dataset
+ * @param metadataMetaClient MetaClient for the MDT (may be null if MDT is
not yet initialized)
+ * @param enabled If true, enable bucketing for MDT partitions;
if false, disable it
+ */
+ public static HoodieTableMetaClient
setMetadataTablePartitionBucketing(HoodieTableMetaClient dataMetaClient,
+
HoodieTableMetaClient metadataMetaClient, boolean enabled) {
+ // Only allowed on the main dataset
+
ValidationUtils.checkArgument(!isMetadataTable(dataMetaClient.getBasePath().toString()),
"Bucketing should only be enabled on the main dataset");
+
+ // Persist on the MDT first, then on the data table. The data table is the
authoritative source for the
+ // retry guard in HoodieBackedTableMetadataWriter#initializeFileGroups
+ // (bucketingEnabled &&
!dataMetaClient...isMetadataTablePartitionBucketingEnabled()). Updating the data
+ // table last ensures that if the MDT update fails the data table stays
un-flipped, so the next retry
+ // naturally re-attempts both updates rather than leaving the two configs
permanently desynced.
+ if (metadataMetaClient != null) {
+ // Persist on MDT so reader code can determine bucketing from MDT props
alone.
+
metadataMetaClient.getTableConfig().setMetadataTablePartitionBucketing(enabled);
+ HoodieTableConfig.update(metadataMetaClient.getStorage(),
metadataMetaClient.getMetaPath(),
metadataMetaClient.getTableConfig().getProps());
+ }
+
+
dataMetaClient.getTableConfig().setMetadataTablePartitionBucketing(enabled);
+ HoodieTableConfig.update(dataMetaClient.getStorage(),
dataMetaClient.getMetaPath(), dataMetaClient.getTableConfig().getProps());
Review Comment:
🤖 The current MDT-first/data-table-second ordering handles the
MDT-update-fails case, but the inverse (MDT update succeeds, data-table
`update` fails or the JVM crashes here) leaves MDT props with `bucketing=true`
while the data table stays `false`. On retry, if FILES has already been
registered or the user changed
`hoodie.metadata.file.group.bucketing.enable=false`, `initializeFileGroups`
falls into the warning path and creates non-bucketed file groups, but readers
still call `isBucketingEnabledForMDT(metadataMetaClient)`→true and look in
`<partition>/<bucket>/`, returning empty. Would it be safer to also clear the
MDT flag in the disable/warning path, or to gate the readers' bucketing
decision on the data-table config? @nsivabalan @yihua could you weigh in on the
right recovery model here?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -1382,6 +1387,22 @@ public Map<String, String> getTableMergeProperties() {
return configs;
}
+ /**
+ * Enables or disables the bucketing for file groups within the metadata
table partitions.
+ *
+ * @param enable Whether to enable or disable bucketing
+ */
+ public void setMetadataTablePartitionBucketing(boolean enable) {
+ setValue(METADATA_TABLE_PARTITION_BUCKETING_ENABLE,
String.valueOf(enable));
+ }
+
+ /**
Review Comment:
🤖 nit: `@returns` isn't a valid javadoc tag — should be `@return`.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java:
##########
@@ -687,6 +687,16 @@ public long getMaxLogFileSize() {
return getLong(MAX_LOG_FILE_SIZE_BYTES_PROP);
}
+ public static final ConfigProperty<Boolean> FILE_GROUP_BUCKETING_ENABLE =
ConfigProperty
+ .key(METADATA_PREFIX + ".file.group.bucketing.enable")
+ .defaultValue(false)
+ .withDocumentation("This flag indicates whether there should be
intermediate partitions under partitions such as record index and filelisting");
Review Comment:
🤖 nit: the doc reads a bit awkwardly ("intermediate partitions under
partitions such as record index and filelisting"). Could you reword to describe
the actual behavior — e.g. "When enabled, MDT file groups are placed in bucket
sub-directories under each metadata partition (record_index, files, ...) to
limit the number of files per directory."
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -374,6 +374,11 @@ public static final String getDefaultPayloadClassName() {
.sinceVersion("1.1.0")
.withDocumentation("This property when set, will define how two versions
of the record will be merged together when records are partially formed");
+ public static final ConfigProperty<String>
METADATA_TABLE_PARTITION_BUCKETING_ENABLE = ConfigProperty
+ .key("hoodie.metadata.partitions.bucketing.enable")
Review Comment:
🤖 nit: the user-facing key here
(`hoodie.metadata.partitions.bucketing.enable`) and the one in
`HoodieMetadataConfig` (`hoodie.metadata.file.group.bucketing.enable`) describe
the same feature with different vocabulary. Could you align them — e.g. both
use `hoodie.metadata.file.group.bucketing.*` — so users don't have to learn two
names for the same concept?
<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]
