This is an automated email from the ASF dual-hosted git repository.
xiangfu0 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 ec52ed81f6f Intern the instance id in ServerInstance (#19583)
ec52ed81f6f is described below
commit ec52ed81f6f3a98adcc40d69dbea81a567b3ce8e
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Wed Sep 16 17:53:48 2026 -0700
Intern the instance id in ServerInstance (#19583)
Co-authored-by: Claude Fable 5.1 <[email protected]>
---
.../routing/manager/BaseBrokerRoutingManager.java | 8 ++++----
.../routing/manager/BrokerRoutingManagerTest.java | 19 +++----------------
.../apache/pinot/core/transport/ServerInstance.java | 10 ++++++++--
.../pinot/core/transport/ServerInstanceTest.java | 12 ++++++++++++
4 files changed, 27 insertions(+), 22 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java
index 4b50b852bed..25e5b784b7d 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java
@@ -416,13 +416,13 @@ public abstract class BaseBrokerRoutingManager implements
RoutingManager, Cluste
String instanceId = instanceConfigZNRecord.getId();
try {
if (isEnabledServer(instanceConfigZNRecord)) {
- // Join Jackson's JVM-interned IS/EV keys; a config-only Guava
interner would use a separate pool.
- instanceId = instanceId.intern();
- enabledServers.add(instanceId);
-
// Always refresh the server instance with the latest instance
config in case it changes
InstanceConfig instanceConfig = new
InstanceConfig(instanceConfigZNRecord);
ServerInstance serverInstance = new ServerInstance(instanceConfig);
+ // Key the maps by the interned instance id so that lookups with the
Jackson-interned instance ids from IS/EV
+ // hit the identity fast path
+ instanceId = serverInstance.getInstanceId();
+ enabledServers.add(instanceId);
if (_enabledServerInstanceMap.put(instanceId, serverInstance) ==
null) {
newEnabledServers.add(instanceId);
diff --git
a/pinot-broker/src/test/java/org/apache/pinot/broker/routing/manager/BrokerRoutingManagerTest.java
b/pinot-broker/src/test/java/org/apache/pinot/broker/routing/manager/BrokerRoutingManagerTest.java
index 5d7830ad04b..82668fd1ce1 100644
---
a/pinot-broker/src/test/java/org/apache/pinot/broker/routing/manager/BrokerRoutingManagerTest.java
+++
b/pinot-broker/src/test/java/org/apache/pinot/broker/routing/manager/BrokerRoutingManagerTest.java
@@ -77,21 +77,8 @@ import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.atLeastOnce;
-import static org.mockito.Mockito.clearInvocations;
-import static org.mockito.Mockito.doThrow;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoInteractions;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
-import static org.mockito.Mockito.when;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertFalse;
-import static org.testng.Assert.assertNotSame;
-import static org.testng.Assert.assertNull;
-import static org.testng.Assert.assertSame;
-import static org.testng.Assert.assertTrue;
+import static org.mockito.Mockito.*;
+import static org.testng.Assert.*;
public class BrokerRoutingManagerTest {
@@ -206,7 +193,7 @@ public class BrokerRoutingManagerTest {
assertEquals(enabledServers.size(), 1);
assertSame(enabledServers.keySet().iterator().next(),
requiredInstanceId);
ServerInstance server = enabledServers.get(requiredInstanceId);
- assertEquals(server.getInstanceId(), SERVER_INSTANCE_ID);
+ assertSame(server.getInstanceId(), requiredInstanceId);
assertEquals(server.getHostname(), SERVER_HOST);
assertEquals(server.getPort(), SERVER_PORT);
// An equal ID on a later config refresh must still replace the server's
configuration.
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java
b/pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java
index 18b296d8adc..b0ca541bf16 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/transport/ServerInstance.java
@@ -59,8 +59,13 @@ public final class ServerInstance {
/// By default (auto joined instances), server instance name is of format:
`Server_<hostname>_<port>`, e.g.
/// `Server_localhost_12345`, hostname is of format: `Server_<hostname>`,
e.g. `Server_localhost`.
+ ///
+ /// The instance id is interned. The same id is repeated as a key across
many long-lived routing structures, and
+ /// Jackson already interns it when decoding the map fields of `IdealState`
/ `ExternalView`, so joining the JVM pool
+ /// lets all of them share one `String` and lets hash lookups hit the
identity fast path. It also keeps the instances
+ /// rebuilt on every instance config refresh from each retaining a separate
copy of the id.
public ServerInstance(InstanceConfig instanceConfig) {
- _instanceId = instanceConfig.getInstanceName();
+ _instanceId = instanceConfig.getInstanceName().intern();
_hostname = extractHostnameFromConfig(instanceConfig);
_port = extractPortFromConfig(instanceConfig);
_grpcPort =
instanceConfig.getRecord().getIntField(Helix.Instance.GRPC_PORT_KEY,
INVALID_PORT);
@@ -113,7 +118,7 @@ public final class ServerInstance {
@VisibleForTesting
ServerInstance(String hostname, int port) {
- _instanceId = Helix.PREFIX_OF_SERVER_INSTANCE + hostname + "_" + port;
+ _instanceId = (Helix.PREFIX_OF_SERVER_INSTANCE + hostname + "_" +
port).intern();
_hostname = hostname;
_port = port;
_grpcPort = INVALID_PORT;
@@ -124,6 +129,7 @@ public final class ServerInstance {
_pool = FALLBACK_POOL_ID;
}
+ /// Returns the interned instance id.
public String getInstanceId() {
return _instanceId;
}
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/transport/ServerInstanceTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/transport/ServerInstanceTest.java
index dbd89f6b29a..e39c3905e89 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/transport/ServerInstanceTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/transport/ServerInstanceTest.java
@@ -24,6 +24,8 @@ import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotSame;
+import static org.testng.Assert.assertSame;
import static org.testng.Assert.assertThrows;
@@ -34,6 +36,16 @@ public class ServerInstanceTest {
.withNonnullFields("_instanceId").verify();
}
+ @Test
+ public void testInstanceIdInterned() {
+ // Build the id at runtime so that the config holds a copy that is not
already in the string pool
+ String instanceId = String.join("_", "Server", "myhost", "1234");
+ assertNotSame(instanceId, "Server_myhost_1234");
+ ServerInstance serverInstance = new ServerInstance(new InstanceConfig(new
ZNRecord(instanceId)));
+ assertSame(serverInstance.getInstanceId(), "Server_myhost_1234");
+ assertSame(new ServerInstance("myhost", 1234).getInstanceId(),
"Server_myhost_1234");
+ }
+
@Test
public void testExtractHostnameFromConfigWithServerPrefix() {
InstanceConfig config = new InstanceConfig(new
ZNRecord("Server_myhost_1234"));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
