You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2023/09/01 17:06:28 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #11486: Remove instances which don't have all segments of a partition in upse…
KKcorps opened a new pull request, #11486:
URL: https://github.com/apache/pinot/pull/11486
This check will help in not sending queries to instances that don't have all the segments for a particular partition in case of upsert tables.
Such scenarios can arise during a table rebalance operation which leads to incorrect results.
--
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
[GitHub] [pinot] KKcorps closed pull request #11486: Fix Segment Assignment and Routing for consuming segments
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps closed pull request #11486: Fix Segment Assignment and Routing for consuming segments
URL: https://github.com/apache/pinot/pull/11486
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703135226
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11486?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11486](https://app.codecov.io/gh/apache/pinot/pull/11486?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (86d1f0e) into [master](https://app.codecov.io/gh/apache/pinot/commit/399f033ec3917df2bc478b5904406a95e0bc7258?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (399f033) will **decrease** coverage by `62.99%`.
> Report is 23 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11486 +/- ##
=============================================
- Coverage 62.99% 0.00% -62.99%
+ Complexity 1094 6 -1088
=============================================
Files 2302 2243 -59
Lines 124025 120712 -3313
Branches 18901 18450 -451
=============================================
- Hits 78126 6 -78120
- Misses 40351 120706 +80355
+ Partials 5548 0 -5548
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11486/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/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-62.86%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-62.99%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11486/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.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11486?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://app.codecov.io/gh/apache/pinot/pull/11486?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (-98.42%)` | :arrow_down: |
... and [1974 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11486/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pinot] KKcorps commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1704667881
> We want to assign new segment based on the existing segment assignment in IS to avoid violating the mapping, but also emit metric when we detect IS and instance partitions are in-sync
This will work for the consuming segments but how should we deal with the table rebalance with 'reassign segments' on e.g. during cluster scale ups.
In that case there's no existing segment assignment
--
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
[GitHub] [pinot] kishoreg commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "kishoreg (via GitHub)" <gi...@apache.org>.
kishoreg commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703860045
@Jackie-Jiang we need protection on both sides. Ensuring IS is always correct is not enough. The routing happens based on the current state and EV and the change is not atomic.
Also, there are many scenarios where a segment on a particular node will become offline or ERROR etc. We dont want be coding for every scenario instead we need to code for the invariant that a server instance should own all the segments belonging to a partition to be eligible to serve the query.
+1 from my side on the idea. I haven't reviewed the code.
--
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
[GitHub] [pinot] KKcorps commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703946700
> @kishoreg Currently we use IS only to determine the segment group (segments that should be routed together). The actual segment state is read from the EV, and we can handle OFFLINE/ERROR etc correctly. What we can potentially improve on the routing side is to read actual ZK metadata to determine the segment group, and after that it should remain the same.
>
> Essentially, the routing side change:
>
> * Is not a must have if we keep IS consistent (all segments of the same partition are assigned to the same set of servers)
> * If IS is inconsistent, this won't be able to fix the query issue, but can return exception instead of potential wrong result
>
> Instead, the assignment side change is a must have, so I'd suggest not do routing side change as of now
Yes, upon having discussion with Kishore offline I also think assignment side change is a must now and not this.
I would focus on the assignment one first.
I think only the logic in `SegmentAssignmentUtils` need to be changed
--
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
[GitHub] [pinot] snleee commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703453007
@Jackie-Jiang So, our new approach should be:
- Block IS update if it violates the instance mapping. Emit metrics + setup alert for observability.
Is my understanding correct? It sounds good to me.
--
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
[GitHub] [pinot] mayankshriv commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "mayankshriv (via GitHub)" <gi...@apache.org>.
mayankshriv commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703305631
>
I think the goal here is to prevent bad query results. If we can prevent that during assignment, let's do that.
--
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
[GitHub] [pinot] snleee commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703355122
Another approach to solve this is to explicitly enforce partition based replica group and make sure that each (replica group, partition) has 1 server.
```
6 servers, 6 partition, 2 replication
(rg_id, partition_id) -> [list of servers]
0, 0 -> server_0
0, 1 -> server_1
0, 2 -> server_2
0, 3 -> server_0
0, 4 -> server_1
0, 5 -> server_2
1, 0 -> server_3
1, 1 -> server_4
1, 2 -> server_5
1, 3 -> server_3
1, 4 -> server_4
1, 5- > server_5
```
This will provide the explicit guarantee for upsert property + protection from the rebalance. Also, we can keep the strict replica group logic as is.
--
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
[GitHub] [pinot] KKcorps commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703283423
> We should not fix this on the routing side, but on the assignment side. The assignment side should ensure all the segments are assigned to the same server when updating the IS. This guarantee might be violated now when IS doesn't align with instance partitions (either because new server added, or rebalance didn't go through). The correct fix should be to check the existing segment's assignment, detect mismatch and emit metric to alert, then assign the segment based on the existing segments' assignment.
Makes sense but I think @mayankshriv wanted some way to not return results in case we run into similar situation again. Should we keep this as well 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
[GitHub] [pinot] Jackie-Jiang commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703380265
> Another approach to solve this is to explicitly enforce partition based replica group for upsert tables and make sure that each (replica group, partition) has 1 server.
>
> ```
> 6 servers, 6 partition, 2 replication
>
> (rg_id, partition_id) -> [list of servers]
> 0, 0 -> server_0
> 0, 1 -> server_1
> 0, 2 -> server_2
> 0, 3 -> server_0
> 0, 4 -> server_1
> 0, 5 -> server_2
> 1, 0 -> server_3
> 1, 1 -> server_4
> 1, 2 -> server_5
> 1, 3 -> server_3
> 1, 4 -> server_4
> 1, 5- > server_5
> ```
>
> This will provide the explicit guarantee for upsert property + protection from the rebalance. Also, we can keep the strict replica group logic as is. but, if we go with this route, we will need to go back to all the existing upsert tables and reconfigure the replica group setup.
@snleee That is already here. The trigger for this issue is the instance partitions was updated, but rebalance was aborted without updating the IS. Essentially IS goes in-sync with the instance partitions. We want to gracefully handle it when adding a new segment to the IS
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #11486: Fix Segment Assignment and Routing for consuming segments
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #11486:
URL: https://github.com/apache/pinot/pull/11486#discussion_r1314553176
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/RealtimeSegmentAssignment.java:
##########
@@ -156,9 +160,57 @@ private List<String> assignConsumingSegment(String segmentName, InstancePartitio
// Explicit partition:
// Assign segment to the first instance within the partition.
- for (int replicaGroupId = 0; replicaGroupId < numReplicaGroups; replicaGroupId++) {
- int partitionId = segmentPartitionId % numPartitions;
- instancesAssigned.add(instancePartitions.getInstances(partitionId, replicaGroupId).get(0));
+ Set<String> existingAssignedInstances = new TreeSet<>();
+ boolean isFirstSegment = true;
+
+ // Loop through all segments to find common instances for the same partitionId
+ for (Map.Entry<String, Map<String, String>> entry : currentAssignment.entrySet()) {
+ String existingSegmentName = entry.getKey();
+ int existingSegmentPartitionId =
+ SegmentAssignmentUtils.getRealtimeSegmentPartitionId(existingSegmentName, _tableNameWithType,
+ _helixManager, _partitionColumn);
+
+ if (existingSegmentPartitionId == segmentPartitionId) {
+ Set<String> segmentInstances = entry.getValue().keySet();
+ if (isFirstSegment) {
+ existingAssignedInstances.addAll(segmentInstances);
+ isFirstSegment = false;
+ } else {
+ if (!segmentInstances.equals(existingAssignedInstances)) {
+ _logger.warn("Existing instances in IS do not match for segment: " + existingSegmentName);
+ //TODO: Add Metric to detect that instances in IS are not same for all segments in the same partition
+ }
+ }
+ }
+ }
+
+ if (existingAssignedInstances.size() == 0) {
+ // No existing segment in the same partition, assign instances based on the instance partitions
+ for (int replicaGroupId = 0; replicaGroupId < numReplicaGroups; replicaGroupId++) {
+ int partitionId = segmentPartitionId % numPartitions;
+ instancesAssigned.add(instancePartitions.getInstances(partitionId, replicaGroupId).get(0));
+ }
+ } else {
+ if (existingAssignedInstances.size() != numReplicaGroups) {
+ _logger.warn(
+ "Number of existing instances in IS : {} does not match number of replica-groups: {} for segment: {}",
+ existingAssignedInstances.size(), numReplicaGroups, segmentName);
+ }
+
+ instancesAssigned.addAll(existingAssignedInstances);
Review Comment:
@Jackie-Jiang @snleee Does this look right? Instead of WARN, I will emit a metric but let me know other than that.
I am getting existing instances assigned for the same partition. If there are none, we use the existing logic to get instances from the instance partition.
Otherwise, we simply use the existing instances.
Also a check in the end to see if we had used Instance partitions, would we get the same result or not. If not, raise a metric warning that something is wrong b/w IS and InstancePartition
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703460959
> @Jackie-Jiang So, our new approach should be:
>
> * Block IS update if it violates the instance mapping. Emit metrics + setup alert for observability.
>
> Is my understanding correct? It sounds good to me.
We want to assign new segment based on the existing segment assignment in IS to avoid violating the mapping, but also emit metric when we detect IS and instance partitions are in-sync
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11486:
URL: https://github.com/apache/pinot/pull/11486#discussion_r1313438462
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/StrictReplicaGroupInstanceSelector.java:
##########
@@ -120,6 +123,53 @@ void updateSegmentMaps(IdealState idealState, ExternalView externalView, Set<Str
}
}
+ TableConfig tableConfig = ZKMetadataProvider.getTableConfig(_propertyStore, _tableNameWithType);
+ Map<String, Map<String, Integer>> segmentCountPerInstancePartition = new HashMap<>();
+ Set<String> corruptedInstances = new HashSet<>();
+ Map<String, Set<String>> segmentsPerPartition = new HashMap<>();
+ if (tableConfig != null && tableConfig.isUpsertEnabled()) {
+ for (Map.Entry<String, Set<String>> entry : oldSegmentToOnlineInstancesMap.entrySet()) {
+ String segment = entry.getKey();
+ String partitionId = String.valueOf(new LLCSegmentName(segment).getPartitionGroupId());
+ Set<String> onlineInstances = entry.getValue();
+ segmentsPerPartition.compute(partitionId, (k, v) -> {
+ if (v == null) {
+ v = new HashSet<>();
+ }
+ v.add(segment);
+ return v;
+ });
+
+ for (String instance : onlineInstances) {
+ if (segmentCountPerInstancePartition.containsKey(partitionId)) {
+ Map<String, Integer> instanceCount = segmentCountPerInstancePartition.get(partitionId);
+ if (instanceCount.containsKey(instance)) {
+ instanceCount.put(instance, instanceCount.get(instance) + 1);
+ } else {
+ instanceCount.put(instance, 1);
+ }
+ } else {
+ Map<String, Integer> instanceCount = new HashMap<>();
+ instanceCount.put(instance, 1);
+ segmentCountPerInstancePartition.put(partitionId, instanceCount);
+ }
+ }
+ }
+
+ // for each partition, check whether each instances has all the segments for a partition assigned to it or not
+ // and then filter out the ones that don't have equal number of segments
+ for (Map.Entry<String, Map<String, Integer>> entry : segmentCountPerInstancePartition.entrySet()) {
+ String partitionId = entry.getKey();
+ Map<String, Integer> instanceCount = entry.getValue();
+ int count = segmentsPerPartition.get(partitionId).size();
+ for (Map.Entry<String, Integer> instanceEntry : instanceCount.entrySet()) {
+ if (instanceEntry.getValue() != count) {
+ corruptedInstances.add(instanceEntry.getKey());
Review Comment:
This algorithm won't work properly when some segments are not online in EV, and will wrongly mark instances as corrupted
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/StrictReplicaGroupInstanceSelector.java:
##########
@@ -120,6 +123,53 @@ void updateSegmentMaps(IdealState idealState, ExternalView externalView, Set<Str
}
}
+ TableConfig tableConfig = ZKMetadataProvider.getTableConfig(_propertyStore, _tableNameWithType);
+ Map<String, Map<String, Integer>> segmentCountPerInstancePartition = new HashMap<>();
+ Set<String> corruptedInstances = new HashSet<>();
+ Map<String, Set<String>> segmentsPerPartition = new HashMap<>();
+ if (tableConfig != null && tableConfig.isUpsertEnabled()) {
+ for (Map.Entry<String, Set<String>> entry : oldSegmentToOnlineInstancesMap.entrySet()) {
+ String segment = entry.getKey();
+ String partitionId = String.valueOf(new LLCSegmentName(segment).getPartitionGroupId());
Review Comment:
This can throw exception on uploaded segment
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703316157
To fix it on the routing side, we need to look at IS instead of the online segments map because EV should not be involved for this check. My suggestion is to make the check on the assumption side, so that we can never run into this state.
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #11486: Remove instances which don't have all segments of a partition in upse…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11486:
URL: https://github.com/apache/pinot/pull/11486#issuecomment-1703937804
@kishoreg Currently we use IS only to determine the segment group (segments that should be routed together). The actual segment state is read from the EV, and we can handle OFFLINE/ERROR etc correctly. What we can potentially improve on the routing side is to read actual ZK metadata to determine the segment group, and after that it should remain the same.
Essentially, the routing side change:
- Is not a must have if we keep IS consistent (all segments of the same partition are assigned to the same set of servers)
- If IS is inconsistent, this won't be able to fix the query issue, but can return exception instead of potential wrong result
Instead, the assignment side change is a must have, so I'd suggest not do routing side change as of now
--
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