shubhampatel28 commented on code in PR #13851:
URL: https://github.com/apache/hudi/pull/13851#discussion_r2334474557
##########
hudi-common/src/main/java/org/apache/hudi/metrics/prometheus/PrometheusReporter.java:
##########
@@ -70,23 +103,39 @@ public PrometheusReporter(HoodieMetricsConfig
metricsConfig, MetricRegistry regi
});
}
metricExports = new DropwizardExports(registry, new
LabeledSampleBuilder(labelNames, labelValues));
- this.collectorRegistry = PORT_TO_COLLECTOR_REGISTRY.get(serverPort);
- metricExports.register(collectorRegistry);
+
+ synchronized (PrometheusReporter.class) {
Review Comment:
just double checked the impl for `metricExports.register(collectorRegistry)`
(for which i added as it not being thread safe) but the
`CollectorRegistry.register` also have synchronized internally so i can remove
the
synchronized here.
```
public void register(Collector m) {
List<String> names = collectorNames(m);
assertNoDuplicateNames(m, names);
synchronized (namesCollectorsLock) {
for (String name : names) {
```
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metrics/prometheus/TestPrometheusReporter.java:
##########
@@ -21,36 +21,79 @@
import org.apache.hudi.common.testutils.HoodieTestUtils;
import org.apache.hudi.config.HoodieWriteConfig;
import org.apache.hudi.config.metrics.HoodieMetricsConfig;
+import org.apache.hudi.config.metrics.HoodieMetricsPrometheusConfig;
import org.apache.hudi.metrics.HoodieMetrics;
import org.apache.hudi.metrics.Metrics;
import org.apache.hudi.metrics.MetricsReporterType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.codahale.metrics.MetricRegistry;
import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
import java.util.UUID;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.when;
@ExtendWith(MockitoExtension.class)
public class TestPrometheusReporter {
+ private static final Logger LOG =
LoggerFactory.getLogger(TestPrometheusReporter.class);
+
@Mock
HoodieWriteConfig writeConfig;
@Mock
HoodieMetricsConfig metricsConfig;
HoodieMetrics hoodieMetrics;
Metrics metrics;
+
+ private static final int TEST_PORT = 19090;
+ private static final int TEST_PORT_2 = 19091;
+ private List<PrometheusReporter> reportersToCleanup;
+
+ @BeforeEach
+ void setUp() {
+ reportersToCleanup = new ArrayList<>();
+ }
@AfterEach
void shutdownMetrics() {
if (metrics != null) {
metrics.shutdown();
}
+
+ for (PrometheusReporter reporter : reportersToCleanup) {
+ try {
+ reporter.stop();
+ } catch (Exception e) {
+ LOG.debug("Exception during test cleanup: {}", e.getMessage());
+ }
+ }
+ reportersToCleanup.clear();
+
+ try {
+ Thread.sleep(100);
Review Comment:
oh.. this was added while i was doing some testing for intermittent failing
tests. its not needed with the sync impl.
--
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]
