From 41c8ad34854a39b4af026d9dce96e2543a6d5651 Mon Sep 17 00:00:00 2001 From: psinghnegi Date: Thu, 28 May 2026 07:49:51 +0000 Subject: [PATCH 1/3] Initialize ServerMetrics in PredownloadScheduler for predownload container The PredownloadScheduler runs in a separate predownload container that starts before the main Pinot server. Previously, it only initialized PredownloadMetrics but did not set up the ServerMetrics registry. This caused PredownloadMetrics (which internally depends on ServerMetrics) to silently fall back to the NOOP metrics instance, resulting in no server-level metrics being emitted from the predownload container. This change initializes ServerMetrics via PinotMetricUtils in the initializeMetricsReporter() method. The initialization is wrapped in a try-catch so that if the underlying metrics factory cannot be created (e.g., because a vendor-specific metrics client is not yet available in the predownload lifecycle), the predownload process continues with the default NOOP ServerMetrics rather than crashing. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../server/predownload/PredownloadScheduler.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java b/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java index 558f055f152a..edd26e6a2665 100644 --- a/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java +++ b/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java @@ -38,6 +38,7 @@ import java.util.concurrent.atomic.AtomicInteger; import org.apache.commons.configuration2.PropertiesConfiguration; import org.apache.commons.io.FileUtils; +import org.apache.pinot.common.metrics.ServerMetrics; import org.apache.pinot.common.utils.TarCompressionUtils; import org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory; import org.apache.pinot.server.conf.ServerConf; @@ -46,6 +47,8 @@ import org.apache.pinot.spi.crypt.PinotCrypterFactory; import org.apache.pinot.spi.env.PinotConfiguration; import org.apache.pinot.spi.filesystem.PinotFSFactory; +import org.apache.pinot.spi.metrics.PinotMetricUtils; +import org.apache.pinot.spi.metrics.PinotMetricsRegistry; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.spi.utils.retry.AttemptsExceededException; import org.slf4j.Logger; @@ -163,6 +166,19 @@ void initializeZK() { void initializeMetricsReporter() { LOGGER.info("Initializing metrics reporter"); + try { + ServerConf serverConf = new ServerConf(_pinotConfig); + PinotMetricsRegistry metricsRegistry = PinotMetricUtils.getPinotMetricsRegistry(serverConf.getMetricsConfig()); + ServerMetrics serverMetrics = + new ServerMetrics(serverConf.getMetricsPrefix(), metricsRegistry, serverConf.emitTableLevelMetrics(), + serverConf.getAllowedTablesForEmittingMetrics()); + serverMetrics.initializeGlobalMeters(); + ServerMetrics.register(serverMetrics); + } catch (Throwable t) { + LOGGER.error("Failed to initialize ServerMetrics in predownload container; " + + "continuing with the currently registered ServerMetrics instance", t); + } + _predownloadMetrics = new PredownloadMetrics(); PredownloadStatusRecorder.registerMetrics(_predownloadMetrics); } From 48f7c1c03863cb249a33cfae2b4d5c3dbbae9dae Mon Sep 17 00:00:00 2001 From: psinghnegi Date: Thu, 28 May 2026 10:28:07 +0000 Subject: [PATCH 2/3] Add tests for ServerMetrics initialization in PredownloadScheduler Three test cases covering the initializeMetricsReporter() change: 1. testInitializeMetricsReporterRegistersServerMetrics - verifies that ServerMetrics is properly initialized and registered when the metrics factory is available 2. testInitializeMetricsReporterFallsBackOnFailure - verifies that when PinotMetricUtils throws (e.g., vendor metrics client not ready), the scheduler continues with the NOOP ServerMetrics instance instead of crashing 3. testInitializeMetricsReporterAlwaysCreatesPredownloadMetrics - verifies that PredownloadMetrics is always created and registered regardless of whether ServerMetrics initialization succeeds or fails Co-Authored-By: Claude Opus 4.6 (1M context) --- .../predownload/PredownloadSchedulerTest.java | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/pinot-server/src/test/java/org/apache/pinot/server/predownload/PredownloadSchedulerTest.java b/pinot-server/src/test/java/org/apache/pinot/server/predownload/PredownloadSchedulerTest.java index a5dca3bd8b5e..958e24bcf3cf 100644 --- a/pinot-server/src/test/java/org/apache/pinot/server/predownload/PredownloadSchedulerTest.java +++ b/pinot-server/src/test/java/org/apache/pinot/server/predownload/PredownloadSchedulerTest.java @@ -33,15 +33,19 @@ import org.apache.commons.io.FileUtils; import org.apache.helix.model.InstanceConfig; import org.apache.pinot.common.metadata.segment.SegmentZKMetadata; +import org.apache.pinot.common.metrics.ServerMetrics; import org.apache.pinot.common.utils.TarCompressionUtils; import org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory; import org.apache.pinot.spi.config.instance.InstanceDataManagerConfig; import org.apache.pinot.spi.config.table.TableConfig; import org.apache.pinot.spi.env.CommonsConfigurationUtils; import org.apache.pinot.spi.filesystem.PinotFSFactory; +import org.apache.pinot.spi.metrics.PinotMetricUtils; +import org.apache.pinot.spi.metrics.PinotMetricsRegistry; import org.mockito.MockedConstruction; import org.mockito.MockedStatic; import org.testng.annotations.AfterClass; +import org.testng.annotations.AfterMethod; import org.testng.annotations.Test; import static org.apache.pinot.server.predownload.PredownloadTestUtil.*; @@ -51,6 +55,8 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; public class PredownloadSchedulerTest { @@ -87,6 +93,11 @@ public void setUp(PropertiesConfiguration properties) _predownloadScheduler._executor = Runnable::run; } + @AfterMethod + public void cleanUpMetrics() { + ServerMetrics.deregister(); + } + @AfterClass public void tearDown() throws Exception { @@ -355,4 +366,68 @@ public void testPredownloadParallelismConfiguration() throws Exception { "Zero parallelism should fall back to default"); zeroScheduler.stop(); } + + @Test + public void testInitializeMetricsReporterRegistersServerMetrics() + throws Exception { + String propertiesFilePath = this.getClass().getClassLoader().getResource(SAMPLE_PROPERTIES_FILE_NAME).getPath(); + PropertiesConfiguration properties = CommonsConfigurationUtils.fromPath(propertiesFilePath); + PredownloadScheduler scheduler = new PredownloadScheduler(properties); + + try (MockedStatic pinotMetricUtilsMockedStatic = mockStatic(PinotMetricUtils.class)) { + PinotMetricsRegistry mockRegistry = mock(PinotMetricsRegistry.class); + pinotMetricUtilsMockedStatic.when(() -> PinotMetricUtils.getPinotMetricsRegistry(any())) + .thenReturn(mockRegistry); + + scheduler.initializeMetricsReporter(); + + pinotMetricUtilsMockedStatic.verify(() -> PinotMetricUtils.getPinotMetricsRegistry(any()), times(1)); + ServerMetrics registeredMetrics = ServerMetrics.get(); + assertNotNull(registeredMetrics, "ServerMetrics should be registered after initializeMetricsReporter"); + } finally { + scheduler.stop(); + } + } + + @Test + public void testInitializeMetricsReporterFallsBackOnFailure() + throws Exception { + String propertiesFilePath = this.getClass().getClassLoader().getResource(SAMPLE_PROPERTIES_FILE_NAME).getPath(); + PropertiesConfiguration properties = CommonsConfigurationUtils.fromPath(propertiesFilePath); + PredownloadScheduler scheduler = new PredownloadScheduler(properties); + + try (MockedStatic pinotMetricUtilsMockedStatic = mockStatic(PinotMetricUtils.class)) { + pinotMetricUtilsMockedStatic.when(() -> PinotMetricUtils.getPinotMetricsRegistry(any())) + .thenThrow(new RuntimeException("Metrics factory not available")); + + scheduler.initializeMetricsReporter(); + + ServerMetrics registeredMetrics = ServerMetrics.get(); + assertNotNull(registeredMetrics, "ServerMetrics.get() should return NOOP instance, not null"); + } finally { + scheduler.stop(); + } + } + + @Test + public void testInitializeMetricsReporterAlwaysCreatesPredownloadMetrics() + throws Exception { + String propertiesFilePath = this.getClass().getClassLoader().getResource(SAMPLE_PROPERTIES_FILE_NAME).getPath(); + PropertiesConfiguration properties = CommonsConfigurationUtils.fromPath(propertiesFilePath); + PredownloadScheduler scheduler = spy(new PredownloadScheduler(properties)); + + try (MockedStatic pinotMetricUtilsMockedStatic = mockStatic(PinotMetricUtils.class); + MockedStatic statusRecorderMockedStatic = + mockStatic(PredownloadStatusRecorder.class)) { + pinotMetricUtilsMockedStatic.when(() -> PinotMetricUtils.getPinotMetricsRegistry(any())) + .thenThrow(new RuntimeException("Metrics factory not available")); + + scheduler.initializeMetricsReporter(); + + statusRecorderMockedStatic.verify( + () -> PredownloadStatusRecorder.registerMetrics(any(PredownloadMetrics.class)), times(1)); + } finally { + scheduler.stop(); + } + } } From 25327c75c22c77468969552740d2246eea95f660 Mon Sep 17 00:00:00 2001 From: psinghnegi Date: Fri, 29 May 2026 03:32:44 +0000 Subject: [PATCH 3/3] Address review comments: narrow catch to Exception, log register result - Changed catch(Throwable) to catch(Exception) to avoid swallowing JVM-level Errors like OutOfMemoryError - Check ServerMetrics.register() return value and log success or error if an instance was already registered --- .../server/predownload/PredownloadScheduler.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java b/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java index edd26e6a2665..eaf4ed7cac7c 100644 --- a/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java +++ b/pinot-server/src/main/java/org/apache/pinot/server/predownload/PredownloadScheduler.java @@ -173,10 +173,15 @@ void initializeMetricsReporter() { new ServerMetrics(serverConf.getMetricsPrefix(), metricsRegistry, serverConf.emitTableLevelMetrics(), serverConf.getAllowedTablesForEmittingMetrics()); serverMetrics.initializeGlobalMeters(); - ServerMetrics.register(serverMetrics); - } catch (Throwable t) { + boolean registered = ServerMetrics.register(serverMetrics); + if (registered) { + LOGGER.info("ServerMetrics successfully registered for predownload container"); + } else { + LOGGER.error("Failed to register ServerMetrics; an instance was already registered"); + } + } catch (Exception e) { LOGGER.error("Failed to initialize ServerMetrics in predownload container; " - + "continuing with the currently registered ServerMetrics instance", t); + + "continuing with the currently registered ServerMetrics instance", e); } _predownloadMetrics = new PredownloadMetrics();