[bugfix] Unavailable instance should not be added to instanceToSegmentsMap in strictReplicaGroup#10302
[bugfix] Unavailable instance should not be added to instanceToSegmentsMap in strictReplicaGroup#1030261yao wants to merge 4 commits intoapache:masterfrom
Conversation
| @Nullable | ||
| private List<String> calculateEnabledInstancesForSegment(String segment, List<String> onlineInstancesForSegment, | ||
| Set<String> unavailableSegments) { | ||
| Set<String> unavailableSegments, String debugString) { |
There was a problem hiding this comment.
Can we avoid addingdebugString? Instead, we can log whether it's from assignment/instance change from the caller. Then, we should be able to check the logs to find the same information.
LOGGER.info("Calculating enabled instances on assignment change");
List<String> enabledInstancesForSegment =
calculateEnabledInstancesForSegment(segment, entry.getValue(), unavailableSegments);
There was a problem hiding this comment.
If I do log info, we will have too much logs since we log one entry even when there is no error?
There was a problem hiding this comment.
Oh this is getting called within the loop. You can add the log before the loop starts?
There was a problem hiding this comment.
I think that is also a lot of logging because we don't need the log when enabledInstancesForSegment is not empty?
| onlineInstances.add(instance); | ||
| instanceToSegmentsMap.computeIfAbsent(instance, k -> new ArrayList<>()).add(segment); | ||
| } | ||
| if (!onlineInstances.isEmpty()) { |
There was a problem hiding this comment.
Let's add the TODO comment about ignoring for the new segments only?
Codecov Report
@@ Coverage Diff @@
## master #10302 +/- ##
============================================
+ Coverage 70.36% 70.42% +0.05%
+ Complexity 5827 5825 -2
============================================
Files 2016 2016
Lines 109635 109651 +16
Branches 16680 16682 +2
============================================
+ Hits 77145 77218 +73
+ Misses 27073 27011 -62
- Partials 5417 5422 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
3 participants
No description provided.