Handling of empty server tags in controller's availability check #13954
+82
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anandheritage
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13954 +/- ##
============================================
- Coverage 61.75% 57.89% -3.86%
- Complexity 207 219 +12
============================================
Files 2436 2612 +176
Lines 133233 143206 +9973
Branches 20636 21988 +1352
============================================
+ Hits 82274 82905 +631
- Misses 44911 53818 +8907
- Partials 6048 6483 +435
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mayankshriv
reviewed
Sep 9, 2024
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java
Outdated
Show resolved
Hide resolved
...test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java
Outdated
Show resolved
Hide resolved
mayankshriv
approved these changes
Sep 9, 2024
Jackie-Jiang
reviewed
Sep 9, 2024
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
anandheritage
changed the title
|
Looked at the failed test cases - seems like flanky ones , doesn't look related to the changes too - In local same tests are working fine. |
Jackie-Jiang
approved these changes
Sep 10, 2024
| @@ -552,6 +565,17 @@ public static List<InstanceConfig> getInstancesConfigsWithTag(List<InstanceConfi | |||
| return instancesWithTag; | |||
| } | |||
|
|
|||
| public static List<InstanceConfig> getInstancesConfigsWithoutTag(List<InstanceConfig> instanceConfigs, String tag) { | |||
There was a problem hiding this comment.
Suggest changing tag to defaultTag and add some javadoc explaining this is the tag representing that there is no tag associated with the instance. Same for other methods
| List<InstanceConfig> instancesWithoutTag = getInstancesConfigsWithoutTag(instanceConfigs, tag); | ||
| return instancesWithoutTag.stream().map(InstanceConfig::getInstanceName).collect(Collectors.toList()); | ||
| } | ||
|
|
There was a problem hiding this comment.
(minor) Remove extra empty line
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR addresses an issue where the Pinot controller incorrectly interprets servers with empty tag lists as "tagged" servers. The current logic in PinotHelixResourceManager checks for the presence of the "server_untagged" tag to determine if a server is untagged and thus available. However, when a server has no tags, it is mistakenly considered as tagged, leading to incorrect behavior in server availability checks.
https://apache-pinot.slack.com/archives/C011C9JHN7R/p1725801169774139