yihua commented on code in PR #11131:
URL: https://github.com/apache/hudi/pull/11131#discussion_r1593525459
##########
hudi-common/src/main/java/org/apache/hudi/metrics/Metrics.java:
##########
@@ -99,8 +97,7 @@ public static synchronized void shutdownAllMetrics() {
private List<MetricsReporter>
addAdditionalMetricsExporters(HoodieMetricsConfig metricConfig) {
List<MetricsReporter> reporterList = new ArrayList<>();
List<String> propPathList =
StringUtils.split(metricConfig.getMetricReporterFileBasedConfigs(), ",");
- try (HoodieStorage storage = HoodieStorageUtils.getStorage(
- propPathList.get(0), HadoopFSUtils.getStorageConf(new
Configuration()))) {
+ try (HoodieStorage storage =
HoodieStorageUtils.getStorage(propPathList.get(0))) {
Review Comment:
Separate this change out? Also, should it get the actual config from
somewhere else? An empty config may not work in some cases (reach out to the
author who made the changes in `Metrics` class).
##########
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestDataGenerator.java:
##########
@@ -578,8 +576,8 @@ private static void createMetadataFile(String f, String
basePath, StorageConfigu
basePath + "/" + HoodieTableMetaClient.METAFOLDER_NAME + "/" + f);
OutputStream os = null;
try {
- FileSystem fs = HadoopFSUtils.getFs(basePath, configuration);
- os = fs.create(commitFile, true);
+ HoodieStorage storage = HoodieStorageUtils.getStorage(basePath,
configuration);
Review Comment:
Changes here can be in an independent PR.
##########
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestUtils.java:
##########
@@ -198,24 +199,25 @@ public static HoodieTableMetaClient init(String basePath,
HoodieTableType tableT
}
/**
- * @param storageConf storage configuration.
+ * @param conf storage configuration.
* @param basePath base path of the Hudi table.
* @return a new {@link HoodieTableMetaClient} instance.
*/
- public static HoodieTableMetaClient createMetaClient(StorageConfiguration<?>
storageConf,
+ public static HoodieTableMetaClient createMetaClient(Configuration conf,
String basePath) {
return HoodieTableMetaClient.builder()
- .setConf(storageConf).setBasePath(basePath).build();
+
.setConf(HoodieStorageUtils.getStorageConf(conf)).setBasePath(basePath).build();
Review Comment:
This needs to use `HadoopFSUtils.getStorageConfWithCopy(conf)` to be
consistent with previous behavior.
##########
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestUtils.java:
##########
@@ -198,24 +199,25 @@ public static HoodieTableMetaClient init(String basePath,
HoodieTableType tableT
}
/**
- * @param storageConf storage configuration.
+ * @param conf storage configuration.
* @param basePath base path of the Hudi table.
* @return a new {@link HoodieTableMetaClient} instance.
*/
- public static HoodieTableMetaClient createMetaClient(StorageConfiguration<?>
storageConf,
+ public static HoodieTableMetaClient createMetaClient(Configuration conf,
String basePath) {
return HoodieTableMetaClient.builder()
- .setConf(storageConf).setBasePath(basePath).build();
+
.setConf(HoodieStorageUtils.getStorageConf(conf)).setBasePath(basePath).build();
Review Comment:
Let's strictly follow the same behavior, even if there may not be obvious or
bug. Any behavior change should be in separate PRs for a closer look.
--
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]
