This is an automated email from the ASF dual-hosted git repository.
shauryachats pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new bb83fa618c2 Initialize ServerMetrics in PredownloadScheduler (#18608)
bb83fa618c2 is described below
commit bb83fa618c22d4d59d838e5ec8429d541d7da90f
Author: Pradeep Singh Negi <[email protected]>
AuthorDate: Tue Jul 7 16:18:30 2026 +0530
Initialize ServerMetrics in PredownloadScheduler (#18608)
* 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) <[email protected]>
* 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) <[email protected]>
* 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
* Extract shared ServerMetricsInitUtils for ServerMetrics
construction/registration
Deduplicates ServerMetrics construction + registration logic between
BaseServerStarter (real server) and PredownloadScheduler (predownload
container). Registration failure now throws IllegalStateException from
the shared method; PredownloadScheduler's existing try/catch keeps
predownload's graceful-degradation behavior unchanged.
* Trigger CI re-run
* Make ServerMetrics registration idempotent to fix integration tests
ServerMetricsInitUtils.initServerMetrics() previously threw
IllegalStateException when ServerMetrics was already registered.
This broke integration tests where multiple servers start in the
same JVM via ClusterTest.startOneServer, causing 38 test failures.
Now returns the existing instance instead of throwing.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
---------
Co-authored-by: psinghnegi <[email protected]>
Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
---
.../server/predownload/PredownloadScheduler.java | 9 +++
.../server/starter/ServerMetricsInitUtils.java | 50 +++++++++++++++
.../server/starter/helix/BaseServerStarter.java | 12 +---
.../predownload/PredownloadSchedulerTest.java | 74 ++++++++++++++++++++++
.../server/starter/ServerMetricsInitUtilsTest.java | 65 +++++++++++++++++++
5 files changed, 201 insertions(+), 9 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 89fa99effd7..79e395760e8 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
@@ -45,6 +45,7 @@ import org.apache.commons.io.FileUtils;
import org.apache.pinot.common.utils.TarCompressionUtils;
import org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory;
import org.apache.pinot.server.conf.ServerConf;
+import org.apache.pinot.server.starter.ServerMetricsInitUtils;
import org.apache.pinot.server.starter.helix.HelixInstanceDataManagerConfig;
import org.apache.pinot.spi.config.instance.InstanceDataManagerConfig;
import org.apache.pinot.spi.crypt.PinotCrypterFactory;
@@ -182,6 +183,14 @@ public class PredownloadScheduler {
void initializeMetricsReporter() {
LOGGER.info("Initializing metrics reporter");
+ try {
+ ServerConf serverConf = new ServerConf(_pinotConfig);
+ ServerMetricsInitUtils.initServerMetrics(serverConf);
+ } catch (Exception e) {
+ LOGGER.error("Failed to initialize ServerMetrics in predownload
container; "
+ + "continuing with the currently registered ServerMetrics instance",
e);
+ }
+
_predownloadMetrics = new PredownloadMetrics();
PredownloadStatusRecorder.registerMetrics(_predownloadMetrics);
}
diff --git
a/pinot-server/src/main/java/org/apache/pinot/server/starter/ServerMetricsInitUtils.java
b/pinot-server/src/main/java/org/apache/pinot/server/starter/ServerMetricsInitUtils.java
new file mode 100644
index 00000000000..0ec8d8b053d
--- /dev/null
+++
b/pinot-server/src/main/java/org/apache/pinot/server/starter/ServerMetricsInitUtils.java
@@ -0,0 +1,50 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.server.starter;
+
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.server.conf.ServerConf;
+import org.apache.pinot.spi.metrics.PinotMetricUtils;
+import org.apache.pinot.spi.metrics.PinotMetricsRegistry;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public final class ServerMetricsInitUtils {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ServerMetricsInitUtils.class);
+
+ private ServerMetricsInitUtils() {
+ }
+
+ public static ServerMetrics initServerMetrics(ServerConf serverConf)
+ throws Exception {
+ PinotMetricsRegistry metricsRegistry =
PinotMetricUtils.getPinotMetricsRegistry(serverConf.getMetricsConfig());
+ ServerMetrics serverMetrics = new
ServerMetrics(serverConf.getMetricsPrefix(), metricsRegistry,
+ serverConf.emitTableLevelMetrics(),
serverConf.getAllowedTablesForEmittingMetrics());
+ serverMetrics.initializeGlobalMeters();
+ boolean registered = ServerMetrics.register(serverMetrics);
+ if (registered) {
+ LOGGER.info("ServerMetrics successfully registered");
+ return serverMetrics;
+ } else {
+ LOGGER.warn("ServerMetrics already registered; returning existing
instance");
+ return ServerMetrics.get();
+ }
+ }
+}
diff --git
a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
index 06febd192c1..66b9d047f3e 100644
---
a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
+++
b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
@@ -114,6 +114,7 @@ import org.apache.pinot.server.conf.ServerConf;
import org.apache.pinot.server.realtime.ControllerLeaderLocator;
import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
import org.apache.pinot.server.starter.ServerInstance;
+import org.apache.pinot.server.starter.ServerMetricsInitUtils;
import org.apache.pinot.server.starter.ServerQueriesDisabledTracker;
import org.apache.pinot.server.worker.WorkerQueryServer;
import org.apache.pinot.spi.accounting.ThreadAccountant;
@@ -126,8 +127,6 @@ import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
import
org.apache.pinot.spi.environmentprovider.PinotEnvironmentProviderFactory;
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.plugin.PluginManager;
import org.apache.pinot.spi.services.ServiceRole;
import org.apache.pinot.spi.services.ServiceStartable;
@@ -678,14 +677,9 @@ public abstract class BaseServerStarter implements
ServiceStartable {
LOGGER.info("Initializing server metrics");
ServerConf serverConf = new ServerConf(_serverConf);
- PinotMetricsRegistry metricsRegistry =
PinotMetricUtils.getPinotMetricsRegistry(serverConf.getMetricsConfig());
- _serverMetrics =
- new ServerMetrics(serverConf.getMetricsPrefix(), metricsRegistry,
serverConf.emitTableLevelMetrics(),
- serverConf.getAllowedTablesForEmittingMetrics());
- _serverMetrics.initializeGlobalMeters();
+ _serverMetrics = ServerMetricsInitUtils.initServerMetrics(serverConf);
_serverMetrics.setValueOfGlobalGauge(ServerGauge.VERSION,
PinotVersion.VERSION_METRIC_NAME, 1);
- ServerMetrics.register(_serverMetrics);
- MseMetrics.registerFromConfig(_serverConf, metricsRegistry);
+ MseMetrics.registerFromConfig(_serverConf,
_serverMetrics.getMetricsRegistry());
LOGGER.info("Initializing reload job status cache");
_reloadJobStatusCache = new ServerReloadJobStatusCache(_instanceId);
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 7432681d289..02e89aa7242 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
@@ -35,17 +35,21 @@ import
org.apache.commons.configuration2.PropertiesConfiguration;
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.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.spi.utils.retry.AttemptsExceededException;
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.*;
@@ -57,6 +61,7 @@ import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.*;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertThrows;
import static org.testng.Assert.assertTrue;
@@ -95,6 +100,11 @@ public class PredownloadSchedulerTest {
_predownloadScheduler._executor = Runnable::run;
}
+ @AfterMethod
+ public void cleanUpMetrics() {
+ ServerMetrics.deregister();
+ }
+
@AfterClass
public void tearDown()
throws Exception {
@@ -365,6 +375,70 @@ public class PredownloadSchedulerTest {
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<PinotMetricUtils> 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<PinotMetricUtils> 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<PinotMetricUtils> pinotMetricUtilsMockedStatic =
mockStatic(PinotMetricUtils.class);
+ MockedStatic<PredownloadStatusRecorder> 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();
+ }
+ }
+
// ── Peer download tests
────────────────────────────────────────────────────
private PredownloadScheduler
buildPeerEnabledScheduler(PropertiesConfiguration properties)
diff --git
a/pinot-server/src/test/java/org/apache/pinot/server/starter/ServerMetricsInitUtilsTest.java
b/pinot-server/src/test/java/org/apache/pinot/server/starter/ServerMetricsInitUtilsTest.java
new file mode 100644
index 00000000000..e16330a0066
--- /dev/null
+++
b/pinot-server/src/test/java/org/apache/pinot/server/starter/ServerMetricsInitUtilsTest.java
@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.server.starter;
+
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.server.conf.ServerConf;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+import static
org.apache.pinot.spi.utils.CommonConstants.CONFIG_OF_METRICS_FACTORY_CLASS_NAME;
+import static
org.apache.pinot.spi.utils.CommonConstants.Server.METRICS_CONFIG_PREFIX;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertSame;
+
+
+public class ServerMetricsInitUtilsTest {
+
+ @AfterMethod
+ public void tearDown() {
+ ServerMetrics.deregister();
+ }
+
+ private static ServerConf buildServerConf() {
+ PinotConfiguration pinotConfig = new PinotConfiguration();
+ pinotConfig.setProperty(METRICS_CONFIG_PREFIX + "." +
CONFIG_OF_METRICS_FACTORY_CLASS_NAME,
+ "org.apache.pinot.plugin.metrics.yammer.YammerMetricsFactory");
+ return new ServerConf(pinotConfig);
+ }
+
+ @Test
+ public void testInitServerMetricsConstructsAndRegisters()
+ throws Exception {
+ ServerMetrics serverMetrics =
ServerMetricsInitUtils.initServerMetrics(buildServerConf());
+
+ assertNotNull(serverMetrics);
+ assertSame(ServerMetrics.get(), serverMetrics, "Returned ServerMetrics
should be the registered instance");
+ }
+
+ @Test
+ public void testInitServerMetricsReturnsExistingWhenAlreadyRegistered()
+ throws Exception {
+ ServerConf serverConf = buildServerConf();
+ ServerMetrics first = ServerMetricsInitUtils.initServerMetrics(serverConf);
+ ServerMetrics second =
ServerMetricsInitUtils.initServerMetrics(serverConf);
+
+ assertSame(second, first, "Should return the already-registered instance");
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
