hudi-agent commented on code in PR #8338:
URL: https://github.com/apache/hudi/pull/8338#discussion_r3565804558
##########
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/sink/ITTestDataStreamWrite.java:
##########
@@ -171,6 +178,38 @@ public void testWriteMergeOnReadWithCompaction(String
indexType) throws Exceptio
testWriteToHoodie(conf, "mor_write_with_compact", 1, EXPECTED);
}
+ @Test
+ public void testVerifyConsistencyOfBucketNum() throws Exception {
+ String path = tempFile.getAbsolutePath();
+ Configuration conf = TestConfigurations.getDefaultConf(path);
+ conf.setString(FlinkOptions.INDEX_TYPE, "BUCKET");
+ conf.setInteger(FlinkOptions.BUCKET_INDEX_NUM_BUCKETS, 4);
+ conf.setInteger(FlinkOptions.COMPACTION_DELTA_COMMITS, 1);
+ conf.setString(FlinkOptions.TABLE_TYPE,
HoodieTableType.MERGE_ON_READ.name());
+
+ OperatorCoordinator.Context context = new
MockOperatorCoordinatorContext(new OperatorID(), 2);
+ StreamWriteOperatorCoordinator coordinator = new
StreamWriteOperatorCoordinator(
+ conf, context);
+ coordinator.start();
+ final org.apache.hadoop.conf.Configuration hadoopConf =
HadoopConfigurations.getHadoopConf(new Configuration());
+ try (FileSystem fs = HadoopFSUtils.getFs(path, hadoopConf)) {
+ assertTrue(fs.exists(new org.apache.hadoop.fs.Path(path,
HoodieTableMetaClient.METAFOLDER_NAME)));
+ }
+ coordinator.close();
+
+ conf.setInteger(FlinkOptions.BUCKET_INDEX_NUM_BUCKETS, 5);
+ coordinator = new StreamWriteOperatorCoordinator(
+ conf, context);
+ try {
+ coordinator.start();
+ } catch (IllegalArgumentException e) {
Review Comment:
π€ This test passes even if `start()` never throws β the catch block is only
reached when the exception fires. Could you add a `fail(...)` right after
`coordinator.start()` so a regression that skips the validation is actually
caught? Otherwise the assertion inside the catch may never run.
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/StreamerUtil.java:
##########
@@ -262,19 +263,52 @@ public static HoodieTableMetaClient initTableIfNotExists(
.setCDCEnabled(conf.getBoolean(FlinkOptions.CDC_ENABLED))
.setCDCSupplementalLoggingMode(conf.getString(FlinkOptions.SUPPLEMENTAL_LOGGING_MODE))
.setTimelineLayoutVersion(1)
+ .setHoodieIndexConf(OptionsResolver.getHoodieIndexConf(conf))
.initTable(hadoopConf, basePath);
LOG.info("Table initialized under base path {}", basePath);
} else {
LOG.info("Table [path={}, name={}] already exists, no need to initialize
the table",
basePath, conf.getString(FlinkOptions.TABLE_NAME));
}
- return StreamerUtil.createMetaClient(conf, hadoopConf);
-
+ HoodieTableMetaClient client = StreamerUtil.createMetaClient(basePath,
hadoopConf);
+ validateTableConfig(conf, client.getTableConfig());
+ return client;
// Do not close the filesystem in order to use the CACHE,
// some filesystems release the handles in #close method.
}
+ /**
+ * Detects conflicts between new parameters and existing table
configurations.
+ */
+ public static void validateTableConfig(Configuration conf, HoodieTableConfig
storedTableConf) {
+ // Should verify the consistency of bucket num at job startup.
+ if (storedTableConf.contains(FlinkOptions.INDEX_TYPE.key())) {
+ HoodieIndex.IndexType storedConfIndexType =
+
HoodieIndex.IndexType.valueOf(storedTableConf.getString(FlinkOptions.INDEX_TYPE.key()));
+ if (conf.contains(FlinkOptions.INDEX_TYPE)) {
+ HoodieIndex.IndexType indexType = OptionsResolver.getIndexType(conf);
+ ValidationUtils.checkArgument(indexType.equals(storedConfIndexType),
+ String.format("Config [%s] = [%s] is conflict with stored
config [%s].",
+ FlinkOptions.INDEX_TYPE.key(),
+ indexType,
+ storedConfIndexType));
+ }
+ if (storedConfIndexType.equals(HoodieIndex.IndexType.BUCKET)
Review Comment:
π€ Does this equality check hold for the consistent-hashing bucket engine?
For `CONSISTENT_HASHING`, `BUCKET_INDEX_NUM_BUCKETS` is only the *initial*
bucket count β the actual bucket number is managed by hashing metadata and can
be resized via clustering. Enforcing strict equality here would reject a
legitimately resized consistent-hashing table. Could you gate this check on
`BUCKET_INDEX_ENGINE_TYPE == SIMPLE`?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/configuration/OptionsResolver.java:
##########
@@ -135,6 +136,16 @@ public static String getPreCombineField(Configuration
conf) {
return preCombineField.equals(FlinkOptions.NO_PRE_COMBINE) ? null :
preCombineField;
}
+ public static Properties getHoodieIndexConf(Configuration conf) {
+ Properties pro = new Properties();
+ String indexType = conf.getString(FlinkOptions.INDEX_TYPE);
Review Comment:
π€ nit: `pro` is pretty cryptic here β could you rename it to `props` or
`indexProps` to make it clearer at a glance?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/StreamerUtil.java:
##########
@@ -262,19 +263,52 @@ public static HoodieTableMetaClient initTableIfNotExists(
.setCDCEnabled(conf.getBoolean(FlinkOptions.CDC_ENABLED))
.setCDCSupplementalLoggingMode(conf.getString(FlinkOptions.SUPPLEMENTAL_LOGGING_MODE))
.setTimelineLayoutVersion(1)
+ .setHoodieIndexConf(OptionsResolver.getHoodieIndexConf(conf))
.initTable(hadoopConf, basePath);
LOG.info("Table initialized under base path {}", basePath);
} else {
LOG.info("Table [path={}, name={}] already exists, no need to initialize
the table",
basePath, conf.getString(FlinkOptions.TABLE_NAME));
}
- return StreamerUtil.createMetaClient(conf, hadoopConf);
-
+ HoodieTableMetaClient client = StreamerUtil.createMetaClient(basePath,
hadoopConf);
+ validateTableConfig(conf, client.getTableConfig());
+ return client;
// Do not close the filesystem in order to use the CACHE,
// some filesystems release the handles in #close method.
}
+ /**
+ * Detects conflicts between new parameters and existing table
configurations.
+ */
+ public static void validateTableConfig(Configuration conf, HoodieTableConfig
storedTableConf) {
+ // Should verify the consistency of bucket num at job startup.
+ if (storedTableConf.contains(FlinkOptions.INDEX_TYPE.key())) {
+ HoodieIndex.IndexType storedConfIndexType =
+
HoodieIndex.IndexType.valueOf(storedTableConf.getString(FlinkOptions.INDEX_TYPE.key()));
+ if (conf.contains(FlinkOptions.INDEX_TYPE)) {
+ HoodieIndex.IndexType indexType = OptionsResolver.getIndexType(conf);
+ ValidationUtils.checkArgument(indexType.equals(storedConfIndexType),
+ String.format("Config [%s] = [%s] is conflict with stored
config [%s].",
+ FlinkOptions.INDEX_TYPE.key(),
+ indexType,
+ storedConfIndexType));
+ }
+ if (storedConfIndexType.equals(HoodieIndex.IndexType.BUCKET)
+ && conf.contains(FlinkOptions.BUCKET_INDEX_NUM_BUCKETS)
+ &&
storedTableConf.contains(FlinkOptions.BUCKET_INDEX_NUM_BUCKETS.key())) {
+ String bucketNums =
conf.get(FlinkOptions.BUCKET_INDEX_NUM_BUCKETS).toString();
+ String storedConfBucketNums =
storedTableConf.getString(FlinkOptions.BUCKET_INDEX_NUM_BUCKETS.key());
+ ValidationUtils.checkArgument(bucketNums.equals(storedConfBucketNums),
+ String.format("Config [%s] = [%s] is not equals stored config
[%s].",
+ HoodieIndexConfig.BUCKET_INDEX_NUM_BUCKETS.key(),
+ bucketNums,
+ storedConfBucketNums));
+ }
+ }
+ // Other config verification.
Review Comment:
π€ nit: `// Other config verification.` doesn't say anything actionable β if
this is a placeholder for future work, could you make it a `// TODO:` comment
(or remove it)?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/StreamerUtil.java:
##########
@@ -262,19 +263,52 @@ public static HoodieTableMetaClient initTableIfNotExists(
.setCDCEnabled(conf.getBoolean(FlinkOptions.CDC_ENABLED))
.setCDCSupplementalLoggingMode(conf.getString(FlinkOptions.SUPPLEMENTAL_LOGGING_MODE))
.setTimelineLayoutVersion(1)
+ .setHoodieIndexConf(OptionsResolver.getHoodieIndexConf(conf))
.initTable(hadoopConf, basePath);
LOG.info("Table initialized under base path {}", basePath);
} else {
LOG.info("Table [path={}, name={}] already exists, no need to initialize
the table",
basePath, conf.getString(FlinkOptions.TABLE_NAME));
}
- return StreamerUtil.createMetaClient(conf, hadoopConf);
-
+ HoodieTableMetaClient client = StreamerUtil.createMetaClient(basePath,
hadoopConf);
+ validateTableConfig(conf, client.getTableConfig());
+ return client;
// Do not close the filesystem in order to use the CACHE,
// some filesystems release the handles in #close method.
}
+ /**
+ * Detects conflicts between new parameters and existing table
configurations.
+ */
+ public static void validateTableConfig(Configuration conf, HoodieTableConfig
storedTableConf) {
+ // Should verify the consistency of bucket num at job startup.
+ if (storedTableConf.contains(FlinkOptions.INDEX_TYPE.key())) {
+ HoodieIndex.IndexType storedConfIndexType =
+
HoodieIndex.IndexType.valueOf(storedTableConf.getString(FlinkOptions.INDEX_TYPE.key()));
+ if (conf.contains(FlinkOptions.INDEX_TYPE)) {
+ HoodieIndex.IndexType indexType = OptionsResolver.getIndexType(conf);
+ ValidationUtils.checkArgument(indexType.equals(storedConfIndexType),
+ String.format("Config [%s] = [%s] is conflict with stored
config [%s].",
Review Comment:
π€ nit: `"is conflict with stored config"` reads awkwardly β maybe
`"conflicts with stored config [%s]"` instead? Same issue on the bucket-num
message a few lines down (`"is not equals stored config"` β `"does not equal
stored config"`). Worth catching now since the integration test asserts on
these exact strings.
<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]
