You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2025/12/19 21:10:58 UTC
[PR] [bugfixing] Fix /debug/routingTable always returning same replica-group (strictReplicaGroup) [pinot]
xiangfu0 opened a new pull request, #17403:
URL: https://github.com/apache/pinot/pull/17403
# Observation
repeated curl `/debug/routingTable/<table>` always returns the same subset of servers even though queries can be evenly distributed.
The table is using `strictReplicaGroup` as routing strategy and table has 2 replicas.
# Root cause
When a table uses `strictReplicaGroup`, the broker chooses a single replica-group based on requestId (e.g. `instanceIdx = requestId % numCandidates`, and the `numCandidates` is 2). This is how Pinot rotates across replica-groups.
However, the broker debug endpoint `/debug/routingTable/{tableName}` was generating a new requestId for each table-type it tried (`OFFLINE` then `REALTIME`). For a realtime-only table called via the raw name (no _REALTIME suffix), the OFFLINE routing call returns null but still consumes the first requestId. That means the REALTIME routing calculation always sees requestId values spaced by 2 (1,3,5,…) which, for an even number of replica-groups (most commonly 2), always maps to the same replica-group index.
This PR fixes the skew by generating a single requestId per `/debug/routingTable` request and reusing it for both `OFFLINE` and `REALTIME` routing computations.
#Tests
Adds `PinotBrokerDebugTest` to verify:
OFFLINE + REALTIME routing in one call use the same requestId
realtime-only raw table calls don’t “skew” REALTIME requestId (REALTIME ids advance by 1 per call, not 2)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfixing] Fix /debug/routingTable always returning same replica-group (strictReplicaGroup) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17403:
URL: https://github.com/apache/pinot/pull/17403
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfixing] Fix /debug/routingTable always returning same replica-group (strictReplicaGroup) [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 closed pull request #17403: [bugfixing] Fix /debug/routingTable always returning same replica-group (strictReplicaGroup)
URL: https://github.com/apache/pinot/pull/17403
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfixing] Fix /debug/routingTable always returning same replica-group (strictReplicaGroup) [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17403:
URL: https://github.com/apache/pinot/pull/17403#discussion_r2636316015
##########
pinot-broker/src/test/java/org/apache/pinot/broker/api/resources/PinotBrokerDebugTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.broker.api.resources;
+
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.List;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.broker.routing.BrokerRoutingManager;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingTable;
+import org.mockito.ArgumentCaptor;
+import org.testng.annotations.Test;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+public class PinotBrokerDebugTest {
+
+ @Test
+ public void testGetRoutingTableUsesSameRequestIdForOfflineAndRealtime()
+ throws Exception {
+ BrokerRoutingManager routingManager = mock(BrokerRoutingManager.class);
+ when(routingManager.getRoutingTable(any(BrokerRequest.class), anyLong()))
+ .thenReturn(new RoutingTable(Collections.emptyMap(), Collections.emptyList(), 0));
+
+ PinotBrokerDebug brokerDebug = new PinotBrokerDebug();
+ Field routingManagerField = PinotBrokerDebug.class.getDeclaredField("_routingManager");
+ routingManagerField.setAccessible(true);
+ routingManagerField.set(brokerDebug, routingManager);
+
+ brokerDebug.getRoutingTable("testTable", (HttpHeaders) null);
+
+ ArgumentCaptor<Long> requestIdCaptor = ArgumentCaptor.forClass(Long.class);
+ verify(routingManager, times(2)).getRoutingTable(any(BrokerRequest.class), requestIdCaptor.capture());
+ List<Long> requestIds = requestIdCaptor.getAllValues();
+ assertEquals(requestIds.size(), 2);
+ assertEquals(requestIds.get(0), requestIds.get(1));
+ }
+
+ @Test
+ public void testGetRoutingTableForRealtimeOnlyRawTableDoesNotSkewRequestId()
+ throws Exception {
+ BrokerRoutingManager routingManager = mock(BrokerRoutingManager.class);
+ when(routingManager.getRoutingTable(any(BrokerRequest.class), anyLong())).thenAnswer(invocation -> {
+ BrokerRequest brokerRequest = invocation.getArgument(0);
+ String tableNameWithType = brokerRequest.getQuerySource().getTableName();
+ if (tableNameWithType.endsWith("_REALTIME")) {
+ return new RoutingTable(Collections.emptyMap(), Collections.emptyList(), 0);
+ }
+ return null;
+ });
+
+ PinotBrokerDebug brokerDebug = new PinotBrokerDebug();
+ Field routingManagerField = PinotBrokerDebug.class.getDeclaredField("_routingManager");
+ routingManagerField.setAccessible(true);
+ routingManagerField.set(brokerDebug, routingManager);
+
+ brokerDebug.getRoutingTable("testTable", (HttpHeaders) null);
+ brokerDebug.getRoutingTable("testTable", (HttpHeaders) null);
+
+ ArgumentCaptor<BrokerRequest> brokerRequestCaptor = ArgumentCaptor.forClass(BrokerRequest.class);
+ ArgumentCaptor<Long> requestIdCaptor = ArgumentCaptor.forClass(Long.class);
+ verify(routingManager, times(4)).getRoutingTable(brokerRequestCaptor.capture(), requestIdCaptor.capture());
+ List<BrokerRequest> brokerRequests = brokerRequestCaptor.getAllValues();
+ List<Long> requestIds = requestIdCaptor.getAllValues();
+
+ assertEquals(brokerRequests.size(), 4);
+ assertEquals(requestIds.size(), 4);
+
+ Long firstRealtimeRequestId = null;
+ Long secondRealtimeRequestId = null;
+ for (int i = 0; i < brokerRequests.size(); i++) {
+ if (brokerRequests.get(i).getQuerySource().getTableName().endsWith("_REALTIME")) {
+ if (firstRealtimeRequestId == null) {
+ firstRealtimeRequestId = requestIds.get(i);
+ } else {
+ secondRealtimeRequestId = requestIds.get(i);
+ }
+ }
+ }
+
+ assertTrue(firstRealtimeRequestId != null);
+ assertTrue(secondRealtimeRequestId != null);
+ assertEquals((long) secondRealtimeRequestId, firstRealtimeRequestId + 1);
Review Comment:
Use `assertEquals(secondRealtimeRequestId, Long.valueOf(firstRealtimeRequestId + 1))` instead of casting to avoid primitive/wrapper confusion. Alternatively, use `assertEquals(secondRealtimeRequestId.longValue(), firstRealtimeRequestId + 1L)` for clearer intent.
```suggestion
assertEquals(secondRealtimeRequestId.longValue(), firstRealtimeRequestId + 1L);
```
##########
pinot-broker/src/test/java/org/apache/pinot/broker/api/resources/PinotBrokerDebugTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.broker.api.resources;
+
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.List;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.pinot.broker.routing.BrokerRoutingManager;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingTable;
+import org.mockito.ArgumentCaptor;
+import org.testng.annotations.Test;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+public class PinotBrokerDebugTest {
+
+ @Test
+ public void testGetRoutingTableUsesSameRequestIdForOfflineAndRealtime()
+ throws Exception {
+ BrokerRoutingManager routingManager = mock(BrokerRoutingManager.class);
+ when(routingManager.getRoutingTable(any(BrokerRequest.class), anyLong()))
+ .thenReturn(new RoutingTable(Collections.emptyMap(), Collections.emptyList(), 0));
+
+ PinotBrokerDebug brokerDebug = new PinotBrokerDebug();
+ Field routingManagerField = PinotBrokerDebug.class.getDeclaredField("_routingManager");
+ routingManagerField.setAccessible(true);
+ routingManagerField.set(brokerDebug, routingManager);
+
+ brokerDebug.getRoutingTable("testTable", (HttpHeaders) null);
+
+ ArgumentCaptor<Long> requestIdCaptor = ArgumentCaptor.forClass(Long.class);
+ verify(routingManager, times(2)).getRoutingTable(any(BrokerRequest.class), requestIdCaptor.capture());
+ List<Long> requestIds = requestIdCaptor.getAllValues();
+ assertEquals(requestIds.size(), 2);
+ assertEquals(requestIds.get(0), requestIds.get(1));
+ }
+
+ @Test
+ public void testGetRoutingTableForRealtimeOnlyRawTableDoesNotSkewRequestId()
+ throws Exception {
+ BrokerRoutingManager routingManager = mock(BrokerRoutingManager.class);
+ when(routingManager.getRoutingTable(any(BrokerRequest.class), anyLong())).thenAnswer(invocation -> {
+ BrokerRequest brokerRequest = invocation.getArgument(0);
+ String tableNameWithType = brokerRequest.getQuerySource().getTableName();
+ if (tableNameWithType.endsWith("_REALTIME")) {
+ return new RoutingTable(Collections.emptyMap(), Collections.emptyList(), 0);
+ }
+ return null;
+ });
+
+ PinotBrokerDebug brokerDebug = new PinotBrokerDebug();
+ Field routingManagerField = PinotBrokerDebug.class.getDeclaredField("_routingManager");
+ routingManagerField.setAccessible(true);
+ routingManagerField.set(brokerDebug, routingManager);
+
+ brokerDebug.getRoutingTable("testTable", (HttpHeaders) null);
+ brokerDebug.getRoutingTable("testTable", (HttpHeaders) null);
+
+ ArgumentCaptor<BrokerRequest> brokerRequestCaptor = ArgumentCaptor.forClass(BrokerRequest.class);
+ ArgumentCaptor<Long> requestIdCaptor = ArgumentCaptor.forClass(Long.class);
+ verify(routingManager, times(4)).getRoutingTable(brokerRequestCaptor.capture(), requestIdCaptor.capture());
+ List<BrokerRequest> brokerRequests = brokerRequestCaptor.getAllValues();
+ List<Long> requestIds = requestIdCaptor.getAllValues();
+
+ assertEquals(brokerRequests.size(), 4);
+ assertEquals(requestIds.size(), 4);
+
+ Long firstRealtimeRequestId = null;
+ Long secondRealtimeRequestId = null;
+ for (int i = 0; i < brokerRequests.size(); i++) {
+ if (brokerRequests.get(i).getQuerySource().getTableName().endsWith("_REALTIME")) {
+ if (firstRealtimeRequestId == null) {
+ firstRealtimeRequestId = requestIds.get(i);
+ } else {
+ secondRealtimeRequestId = requestIds.get(i);
+ }
+ }
+ }
+
+ assertTrue(firstRealtimeRequestId != null);
+ assertTrue(secondRealtimeRequestId != null);
Review Comment:
Replace `assertTrue(variable != null)` with `assertNotNull(variable)` for more idiomatic null checks. This provides clearer error messages and follows TestNG best practices.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfixing] Fix /debug/routingTable always returning same replica-group (strictReplicaGroup) [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17403:
URL: https://github.com/apache/pinot/pull/17403#issuecomment-3676791849
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17403?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.62%. Comparing base ([`bdd8347`](https://app.codecov.io/gh/apache/pinot/commit/bdd834797a7cef5f647b84b9fdf395f3e2678475?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bc37db7`](https://app.codecov.io/gh/apache/pinot/commit/bc37db7b998da881691ba9cc0437d24e53c4f84c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (bdd8347) and HEAD (bc37db7). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (bdd8347) | HEAD (bc37db7) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17403 +/- ##
============================================
- Coverage 63.25% 55.62% -7.63%
+ Complexity 1474 702 -772
============================================
Files 3152 2464 -688
Lines 187861 139082 -48779
Branches 28761 22177 -6584
============================================
- Hits 118833 77366 -41467
+ Misses 59809 55186 -4623
+ Partials 9219 6530 -2689
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.62% <ø> (-7.60%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.62% <ø> (-7.63%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.62% <ø> (-7.63%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.62% <ø> (-0.04%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17403/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17403?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org