nsivabalan commented on code in PR #10635:
URL: https://github.com/apache/hudi/pull/10635#discussion_r1554986358
##########
hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java:
##########
@@ -97,7 +97,7 @@ protected BaseTableMetadata(HoodieEngineContext
engineContext, HoodieMetadataCon
this.isMetadataTableInitialized =
dataMetaClient.getTableConfig().isMetadataTableAvailable();
if (metadataConfig.enableMetrics()) {
- this.metrics = Option.of(new
HoodieMetadataMetrics(Registry.getRegistry("HoodieMetadata")));
+ this.metrics = Option.of(new
HoodieMetadataMetrics(HoodieMetricsConfig.newBuilder().fromProperties(metadataConfig.getProps()).build()));
Review Comment:
metadataConfig is not going to contain any metrics related props. this is on
the reader side.
What we have fixed in HoodieMetadataWriteUtils is applicable for metadata
writer and not reader.
We need some fixes here. if not, the metrics related props may not be
carried over to this code snippet.
##########
hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java:
##########
@@ -97,7 +97,7 @@ protected BaseTableMetadata(HoodieEngineContext
engineContext, HoodieMetadataCon
this.isMetadataTableInitialized =
dataMetaClient.getTableConfig().isMetadataTableAvailable();
if (metadataConfig.enableMetrics()) {
- this.metrics = Option.of(new
HoodieMetadataMetrics(Registry.getRegistry("HoodieMetadata")));
+ this.metrics = Option.of(new
HoodieMetadataMetrics(HoodieMetricsConfig.newBuilder().fromProperties(metadataConfig.getProps()).build()));
Review Comment:
but I am not sure if we can even get that. bcoz, the query engine is not
going to set any writer props (for eg metrics related ones). So, its not
feasible for us to instantiate this properly on the reader side :(
##########
hudi-common/src/main/java/org/apache/hudi/metrics/Metrics.java:
##########
@@ -166,6 +169,17 @@ public void registerGauge(String metricName, final long
value) {
}
}
+ public HoodieGauge<Long> registerGauge(String metricName) {
+ try {
Review Comment:
why can't we call the other method here.
```
registerGauge(String metricName, 0L);
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataWriteUtils.java:
##########
@@ -200,6 +200,11 @@ public static HoodieWriteConfig createMetadataWriteConfig(
builder.withProperties(datadogConfig.build().getProps());
break;
case PROMETHEUS:
+ HoodieMetricsPrometheusConfig prometheusConfig =
HoodieMetricsPrometheusConfig.newBuilder()
+ .withPushgatewayLabels(writeConfig.getPushGatewayLabels())
+ .withPrometheusPortNum(writeConfig.getPrometheusPort()).build();
Review Comment:
why we are not setting other props like host, jobname, etc.
##########
hudi-common/src/main/java/org/apache/hudi/metrics/Metrics.java:
##########
@@ -176,4 +190,16 @@ public static boolean isInitialized(String basePath) {
}
return false;
}
+
+ /**
+ * Use the same base path as the hudi table so that Metrics instance is
shared.
+ */
+ private static String getBasePath(HoodieMetricsConfig metricsConfig) {
+ String basePath = metricsConfig.getBasePath();
+ if (basePath.endsWith(HoodieTableMetaClient.METADATA_TABLE_FOLDER_PATH)) {
Review Comment:
can we introduce a utility for deducing metadata table.
btw, we should check the entire dir name matches "metadata" and not just
ends with. We could possible have a table named "customer_metadata" or
something of those sorts. above check could actually match for this table path.
--
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]
