You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shauryachats (via GitHub)" <gi...@apache.org> on 2026/05/06 22:32:53 UTC
[PR] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
shauryachats opened a new pull request, #18433:
URL: https://github.com/apache/pinot/pull/18433
## Summary
Multi-stream realtime tables encode Pinot partition IDs as `streamIndex * 10000 + streamPartitionId`. Before this fix, RealtimeSegmentAssignment and ReplicaGroupSegmentAssignmentStrategy used the raw (encoded) partition ID directly when computing instance slots, causing incorrect slot mapping and breaking colocation of segments belonging to the same partition.
### Changes:
- In `RealtimeSegmentAssignment.assignConsumingSegment`, decode the Pinot partition ID to the stream-level partition ID via `IngestionConfigUtils.getStreamPartitionIdFromPinotPartitionId` before
computing the instance index.
- In `ReplicaGroupSegmentAssignmentStrategy`, extract a `getPartitionIdFromSegmentName` helper that applies the same decoding for REALTIME tables before `% numPartitions`, fixing both single-segment assignment and rebalance paths.
## Testing
Deployed this on an internal cluster containing a multi-topic table and verified by setting the `instanceAssignmentConfig` as:
```
"instanceAssignmentConfigMap": {
"CONSUMING": {
"tagPoolConfig": {
"tag": "cluster_REALTIME",
"poolBased": false,
"numPools": 0
},
"replicaGroupPartitionConfig": {
"replicaGroupBased": true,
"numInstances": 0,
"numReplicaGroups": 2,
"numInstancesPerReplicaGroup": 3,
"numPartitions": 3,
"numInstancesPerPartition": 1,
"minimizeDataMovement": true,
"partitionColumn": "trace_id"
},
"partitionSelector": "INSTANCE_REPLICA_GROUP_PARTITION_SELECTOR",
"minimizeDataMovement": false
},
"COMPLETED": {
"tagPoolConfig": {
"tag": "cluster_REALTIME",
"poolBased": false,
"numPools": 0
},
"replicaGroupPartitionConfig": {
"replicaGroupBased": true,
"numInstances": 0,
"numReplicaGroups": 2,
"numInstancesPerReplicaGroup": 3,
"numPartitions": 3,
"numInstancesPerPartition": 1,
"minimizeDataMovement": true,
"partitionColumn": "trace_id"
},
"partitionSelector": "INSTANCE_REPLICA_GROUP_PARTITION_SELECTOR",
"minimizeDataMovement": false
}
},
```
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3270302503
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
+ /// that lack stream configs (treat as single-stream).
public static int getStreamPartitionIdFromPinotPartitionId(TableConfig tableConfig, int partitionId) {
- return hasMultipleStreams(tableConfig) ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
+ if (tableConfig.getTableType() != TableType.REALTIME) {
Review Comment:
Why do we ever call this on an OFFLINE table?
Even if we call it on an OFFLINE table, we shouldn't differentiate it from real-time table
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4711233818
Agreed on the revisiting multi-stream design ;)
Speaking of the PR, if we only need to change replica group based assignment, then let's limit the code change and partition assignment logic only into that ReplicaGroupSegmentAssignmentStrategy?
Also we can hide this behind a flag?
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3270986765
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
+ /// that lack stream configs (treat as single-stream).
public static int getStreamPartitionIdFromPinotPartitionId(TableConfig tableConfig, int partitionId) {
- return hasMultipleStreams(tableConfig) ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
Review Comment:
Why are we changing this method? Are they not equivalent?
Also if you're to change them I'd suggest to change the method `hasMultipleStreams` instead, so that this method is clear to return the original partition id whenever it has no multiple stream
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4484337112
@J-HowHuang @noob-se7en @xiangfu0 please review, thanks!
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3251642526
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
Review Comment:
minor: why /// for javadoc? should it be //
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3270295406
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
Review Comment:
`///` is markdown style javadoc. `//` is comment
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4595321711
> In the above example, the current assignment will assign in a round-robin manner to balance segment placement. Why 10000%3 = 1 is a wrong assignment result?
`10000 % 3 = 1` is not a wrong result by itself - if there is no expected partitioning by a field. The distribution is balanced either way - that's a fair point. The correctness issue is specifically about partition-aware query routing, not balance.
When `partitionColumn: "trace_id"` is configured, Pinot uses the instance partition assignment to prune which servers a query needs to hit. A query like `WHERE trace_id = 'abc'` determines which logical partition owns that trace_id, an routes only to the servers holding that partition — skipping the rest. This is the whole point of the replica-group partition configuration.
This pruning only works if all data for the same logical partition (across all streams) lives on the same instance group. In this setup, both streams are co-partitioned by trace_id, so stream 0 partition 0 and stream 1 partition contain data for the exact same set of trace IDs.
Without the fix:
- Stream 0, partition 0 (Pinot ID 0) → 0 % 3 = 0 → instance group 0 (Server A)
- Stream 1, partition 0 (Pinot ID 10000) → 10000 % 3 = 1 → instance group 1 (Server B)
Now a query for a trace_id in logical partition 0 must hit both Server A and Server B, because the data is split across
two instance groups. Partition pruning is broken.
With the fix, both map to instance group 0 (Server A), and partition pruning works as intended.
If the two streams had no relationship between their partition keys, the colocation argument wouldn't apply — but then
the user also wouldn't be configuring a shared partitionColumn across them.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3270988629
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
+ /// that lack stream configs (treat as single-stream).
public static int getStreamPartitionIdFromPinotPartitionId(TableConfig tableConfig, int partitionId) {
- return hasMultipleStreams(tableConfig) ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
+ if (tableConfig.getTableType() != TableType.REALTIME) {
+ return partitionId;
+ }
+ IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
+ if (ingestionConfig == null || ingestionConfig.getStreamIngestionConfig() == null) {
+ return partitionId;
+ }
+ List<Map<String, String>> streamConfigMaps = ingestionConfig.getStreamIngestionConfig().getStreamConfigMaps();
+ return streamConfigMaps != null && streamConfigMaps.size() > 1
+ ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
Review Comment:
Why are we changing this method? Are they not equivalent?
Also if you're to change them I'd suggest to change the method `hasMultipleStreams` instead, so that this method is clear to return the original partition id whenever it has no multiple stream
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4580920166
> > I do not understand this example and the argument. The partitions 0 and 10000 belong to two different streams. Although both of them is the partition 0 of their streams, why do they need to be colocated?
>
> Good question - let me clarify the colocation argument.
>
> In this specific setup, both streams are co-partitioned by the same key (trace_id). That means stream 0 partition 0 and stream 1 partition 0 contain data for the same set of trace IDs (those where trace_id % 3 == 0). Colocating them on the same server means a query filtering by a specific trace_id can be served entirely locally without scatter-gathering across multiple server groups.
>
> That said, you're right that if the two streams have no relationship between their partition keys, colocation across streams wouldn't be a requirement.
>
> But more fundamentally, the fix is necessary for correctness of instance assignment even independent of the colocation argument. With numPartitions: 3 configured, the intent is:
>
> * stream partition 0 → instance group 0
> * stream partition 1 → instance group 1
> * stream partition 2 → instance group 2
>
> Without the fix, stream 1's segments get assigned via the raw Pinot partition ID:
>
> * 10000 % 3 = 1 → instance group 1 (wrong, should be 0)
> * 10001 % 3 = 2 → instance group 2 (wrong, should be 1)
> * 10002 % 3 = 0 → instance group 0 (wrong, should be 2)
In the above example, the current assignment will assign in a round-robin manner to balance segment placement. Why 10000%3 = 1 is a wrong assignment result?
>
> This produces an arbitrary and scrambled mapping that doesn't match what the user configured at all — segments from stream 1 would be distributed across servers in a way that's inconsistent with the replicaGroupPartitionConfig. The fix ensures both streams use their stream-level partition ID consistently when computing the instance group.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3270986765
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
+ /// that lack stream configs (treat as single-stream).
public static int getStreamPartitionIdFromPinotPartitionId(TableConfig tableConfig, int partitionId) {
- return hasMultipleStreams(tableConfig) ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
Review Comment:
Why are we changing this method? Are they not equivalent?
Also if you're to change them I'd suggest to change the method `hasMultipleStreams` instead, so that this method is clear to return the original partition id whenever it has no multiple stream
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3320778060
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
+ /// that lack stream configs (treat as single-stream).
public static int getStreamPartitionIdFromPinotPartitionId(TableConfig tableConfig, int partitionId) {
- return hasMultipleStreams(tableConfig) ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
+ if (tableConfig.getTableType() != TableType.REALTIME) {
+ return partitionId;
+ }
+ IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
+ if (ingestionConfig == null || ingestionConfig.getStreamIngestionConfig() == null) {
+ return partitionId;
+ }
+ List<Map<String, String>> streamConfigMaps = ingestionConfig.getStreamIngestionConfig().getStreamConfigMaps();
+ return streamConfigMaps != null && streamConfigMaps.size() > 1
+ ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
Review Comment:
They were not equivalent for OFFLINE tables — the old hasMultipleStreams delegated to getStreamConfigMaps, which throws `IllegalStateException` for OFFLINE tables (it has a Preconditions.checkState on TableType.REALTIME). Since ReplicaGroupSegmentAssignmentStrategy calls this for both table types, the call would throw on OFFLINE tables.
Agreed with your suggestion: Fixed hasMultipleStreams to safely return false for OFFLINE tables and null
ingestionConfig.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "noob-se7en (via GitHub)" <gi...@apache.org>.
noob-se7en commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4515345135
> The multi-topic partition id overloading existing partition id has caused so many issues:
>
> * We are missing checks here and there
> * Stream with custom partition id fails (See #17260)
>
> I think it is the time to revisit this design decision and avoid the troubles going forward. I can see 2 possible solutions:
>
> * Make `partition_id` part within the table name `x_y` where x is the topic id and y is the partition id. For single topic name, it remains the same
> * Leverage logical table for multiple topics, each topic a separate physical table
(Not related to this PR)
Big +1 on this. The base design itself needs to be improved, the realtime ingestion code has suffered because of it (correctness + quality wise), and reviews are harder as well (e.g., https://github.com/apache/pinot/pull/18175). There is diminishing interest in reviewing further development on multi-topic due to these issues.
Bumping up the issue related to this discussion: https://github.com/apache/pinot/issues/17260
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4392917637
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18433?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.66%. Comparing base ([`5ccd383`](https://app.codecov.io/gh/apache/pinot/commit/5ccd3831b276de03f46565963576378c72c6a54d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`551eb3d`](https://app.codecov.io/gh/apache/pinot/commit/551eb3d33073300d11ae92511d81b9356debebda?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (5ccd383) and HEAD (551eb3d). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (5ccd383) | HEAD (551eb3d) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18433 +/- ##
============================================
- Coverage 63.57% 55.66% -7.91%
+ Complexity 1717 860 -857
============================================
Files 3252 2541 -711
Lines 199132 147683 -51449
Branches 30875 23828 -7047
============================================
- Hits 126596 82210 -44386
+ Misses 62454 58450 -4004
+ Partials 10082 7023 -3059
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18433/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/18433/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/18433/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/18433/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/18433/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/18433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.66% <ø> (-7.91%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.66% <ø> (-7.91%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.66% <ø> (-7.91%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18433/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.66% <ø> (+0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18433/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/18433?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
Re: [PR] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3251649804
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
Review Comment:
Just followed the existing convention. I can make it `//`.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "rseetham (via GitHub)" <gi...@apache.org>.
rseetham commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4556950368
+1 to both suggestions from @Jackie-Jiang and @noob-se7en.
Beyond the correctness and code quality problems already mentioned, there’s another operational concern worth raising: topic deletion.
While the current design does allow deleting a topic in the middle, doing so breaks segment ordering — if the 10000-range partitions are removed, the 20000-range partitions shift down to take their place, causing incorrect segment mapping. This makes mid-sequence topic deletion effectively unsafe in practice. Only deleting the final topic is actually safe. And even then, we have to manually clean up the segments for the topic.
Of the two options, the first (encoding partition_id as x_y) seems preferable. The logical table approach does solve the problem today, but it shifts the burden of managing multiple physical tables onto the user. Having this handled natively would be a much cleaner experience imo.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18433:
URL: https://github.com/apache/pinot/pull/18433#discussion_r3320772851
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/IngestionConfigUtils.java:
##########
@@ -117,8 +117,19 @@ public static int getPinotPartitionIdFromStreamPartitionId(int partitionId, int
}
/// Returns the stream partition id from the Pinot segment partition id.
+ /// Safe to call for any table type: returns `partitionId` unchanged for OFFLINE tables or REALTIME tables
+ /// that lack stream configs (treat as single-stream).
public static int getStreamPartitionIdFromPinotPartitionId(TableConfig tableConfig, int partitionId) {
- return hasMultipleStreams(tableConfig) ? getStreamPartitionIdFromPinotPartitionId(partitionId) : partitionId;
+ if (tableConfig.getTableType() != TableType.REALTIME) {
Review Comment:
`ReplicaGroupSegmentAssignmentStrategy` handles both OFFLINE and REALTIME tables and calls this method unconditionally. You're right that the OFFLINE branch shouldn't live here.
Moved the null/OFFLINE safety into `hasMultipleStreams` so it safely returns false for any non-multi-stream case.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4579885435
> I do not understand this example and the argument. The partitions 0 and 10000 belong to two different streams. Although both of them is the partition 0 of their streams, why do they need to be colocated?
Good question - let me clarify the colocation argument.
In this specific setup, both streams are co-partitioned by the same key (trace_id). That means stream 0 partition 0 and
stream 1 partition 0 contain data for the same set of trace IDs (those where trace_id % 3 == 0). Colocating them on the
same server means a query filtering by a specific trace_id can be served entirely locally without scatter-gathering
across multiple server groups.
That said, you're right that if the two streams have no relationship between their partition keys, colocation across
streams wouldn't be a requirement.
But more fundamentally, the fix is necessary for correctness of instance assignment even independent of the colocation argument. With numPartitions: 3 configured, the intent is:
- stream partition 0 → instance group 0
- stream partition 1 → instance group 1
- stream partition 2 → instance group 2
Without the fix, stream 1's segments get assigned via the raw Pinot partition ID:
- 10000 % 3 = 1 → instance group 1 (wrong, should be 0)
- 10001 % 3 = 2 → instance group 2 (wrong, should be 1)
- 10002 % 3 = 0 → instance group 0 (wrong, should be 2)
This produces an arbitrary and scrambled mapping that doesn't match what the user configured at all — segments from stream 1 would be distributed across servers in a way that's inconsistent with the replicaGroupPartitionConfig. The fix ensures both streams use their stream-level partition ID consistently when computing the instance group.
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat commented on PR #18433:
URL: https://github.com/apache/pinot/pull/18433#issuecomment-4571054731
> For this setup, the raw partition ID 0 and 10000 would map to instance partition 0 % 3 = 0 and 10000 % 3 = 1 without the fix which meant different servers for the same stream partition ID which breaks colocation of data.
I do not understand this example and the argument. The partitions 0 and 10000 belong to two different streams. Although both of them is the partition 0 of their streams, why do they need to be colocated?
--
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] [bugfix] Fixing instance partition assignments for multi-stream realtime tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #18433:
URL: https://github.com/apache/pinot/pull/18433
--
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