-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-7391] HoodieMetadataMetrics should use Metrics instance for metrics registry #10635
Conversation
hey @prashantwason : can you review this patch please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 minor comment
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java
Show resolved
Hide resolved
...ient/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataWriteUtils.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/BaseTableMetadata.java
Show resolved
Hide resolved
@@ -166,6 +169,17 @@ public void registerGauge(String metricName, final long value) { | |||
} | |||
} | |||
|
|||
public HoodieGauge<Long> registerGauge(String metricName) { | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we call the other method here.
registerGauge(String metricName, 0L);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets address this minor comment and we are good to land
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
…rics registry (#10635) Currently HoodieMetadataMetrics stores metrics in memory and these metrics are not pushed by the metric reporters. The metric reporters are configured within Metrics instance. List of changes in the PR: Metrics related classes have been moved from hudi-client-common to hudi-common. HoodieMetadataMetrics now uses Metrics class so that all the reporters can be supported with it. Some gaps in configs which are added in HoodieMetadataWriteUtils Some metrics related apis and functionality has been moved to HoodieMetricsConfig. The HoodieWriteConfig APIs now delegate to HoodieMetricsConfig for the functionality.
…rics registry (#10635) Currently HoodieMetadataMetrics stores metrics in memory and these metrics are not pushed by the metric reporters. The metric reporters are configured within Metrics instance. List of changes in the PR: Metrics related classes have been moved from hudi-client-common to hudi-common. HoodieMetadataMetrics now uses Metrics class so that all the reporters can be supported with it. Some gaps in configs which are added in HoodieMetadataWriteUtils Some metrics related apis and functionality has been moved to HoodieMetricsConfig. The HoodieWriteConfig APIs now delegate to HoodieMetricsConfig for the functionality.
…rics registry (#10635) Currently HoodieMetadataMetrics stores metrics in memory and these metrics are not pushed by the metric reporters. The metric reporters are configured within Metrics instance. List of changes in the PR: Metrics related classes have been moved from hudi-client-common to hudi-common. HoodieMetadataMetrics now uses Metrics class so that all the reporters can be supported with it. Some gaps in configs which are added in HoodieMetadataWriteUtils Some metrics related apis and functionality has been moved to HoodieMetricsConfig. The HoodieWriteConfig APIs now delegate to HoodieMetricsConfig for the functionality.
…rics registry (#10635) Currently HoodieMetadataMetrics stores metrics in memory and these metrics are not pushed by the metric reporters. The metric reporters are configured within Metrics instance. List of changes in the PR: Metrics related classes have been moved from hudi-client-common to hudi-common. HoodieMetadataMetrics now uses Metrics class so that all the reporters can be supported with it. Some gaps in configs which are added in HoodieMetadataWriteUtils Some metrics related apis and functionality has been moved to HoodieMetricsConfig. The HoodieWriteConfig APIs now delegate to HoodieMetricsConfig for the functionality.
…rics registry (#10635) Currently HoodieMetadataMetrics stores metrics in memory and these metrics are not pushed by the metric reporters. The metric reporters are configured within Metrics instance. List of changes in the PR: Metrics related classes have been moved from hudi-client-common to hudi-common. HoodieMetadataMetrics now uses Metrics class so that all the reporters can be supported with it. Some gaps in configs which are added in HoodieMetadataWriteUtils Some metrics related apis and functionality has been moved to HoodieMetricsConfig. The HoodieWriteConfig APIs now delegate to HoodieMetricsConfig for the functionality.
Change Logs
Currently HoodieMetadataMetrics stores metrics in memory and these metrics are not pushed by the metric reporters. The metric reporters are configured within Metrics instance. List of changes in the PR:
Impact
There are constructors and APIs which were using
HoodieWriteConfig
earlier. Now they are usingHoodieMetricsConfig
.HoodieMetricsConfig
can be obtained by callinggetMetricsConfig()
from theHoodieWriteConfig
object.Risk level (write none, low medium or high below)
low
Documentation Update
There are constructors and APIs which were using
HoodieWriteConfig
earlier. Now they are usingHoodieMetricsConfig
.HoodieMetricsConfig
can be obtained by callinggetMetricsConfig()
from theHoodieWriteConfig
object.Contributor's checklist