You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "itschrispeck (via GitHub)" <gi...@apache.org> on 2024/07/04 09:05:28 UTC
[PR] Fix bug when evaluating resource status during service startup check [pinot]
itschrispeck opened a new pull request, #13541:
URL: https://github.com/apache/pinot/pull/13541
We have seen servers sometimes fail to pass the service status checker until the timeout is reached, even after all segments are online/in the expected state. Logs show:
```
Sleep for 10000ms as service status has not turned GOOD: MultipleCallbackServiceStatusCallback:IdealStateAndCurrentStateMatchServiceStatusCallback:Helix state does not exist, waitingFor=CurrentStateMatch, resource=table_REALTIME, numResourcesLeft=2, numTotalResources=802, minStartCount=802,;IdealStateAndExternalViewMatchServiceStatusCallback:Init;;
```
This is due to [this check](https://github.com/apache/pinot/blob/cf1a0f6b9b44fdb567452b63a34e76ac5c635429/pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java#L437-L440), which considers the table resource to have `STARTING` status if the external view/current state is `null`. However, this isn't a valid assumption since the current state can be null if the last segment on the server is removed and the ideal state still exists. We primary see this behavior with completed segment redistribution turned on, on small tables.
The change here is meant to allow the resource status to return `GOOD` if the instance is no longer assigned any segment (when the server first started and collected all resources to monitor it was assigned).
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on PR #13541:
URL: https://github.com/apache/pinot/pull/13541#issuecomment-2249074665
> My bad, now I see the problem. Should we consider first looping over the ideal state and create a map from serving partition to state, then only loop over this map in the second loop? Similar to the current approach but without the early termination in the first loop.
Yeah, I think that is clearer. Updated to use `idealState.getRecord().getMapFields()` as well
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #13541:
URL: https://github.com/apache/pinot/pull/13541#issuecomment-2232071149
My bad, now I see the problem.
Should we consider first looping over the ideal state and create a map from serving partition to state, then only loop over this map in the second loop? Similar to the current approach but without the early termination in the first loop.
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #13541:
URL: https://github.com/apache/pinot/pull/13541#discussion_r1671366153
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java:
##########
@@ -443,8 +467,9 @@ private StatusDescriptionPair evaluateResourceStatus(String resourceName) {
// external view or went to ERROR state (which means that we tried to load the segments/resources but failed for
// some reason)
Map<String, String> partitionStateMap = getPartitionStateMap(helixState);
- for (String partitionName : idealState.getPartitionSet()) {
- String idealStateStatus = idealState.getInstanceStateMap(partitionName).get(_instanceName);
+ for (String partitionName : partitionSet) {
+ String idealStateStatus = partitionToIdealStateStatusMap.getOrDefault(partitionName,
+ idealState.getInstanceStateMap(partitionName).get(_instanceName));
Review Comment:
Got it, make sense.
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #13541:
URL: https://github.com/apache/pinot/pull/13541
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #13541:
URL: https://github.com/apache/pinot/pull/13541#discussion_r1670520235
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java:
##########
@@ -443,8 +467,9 @@ private StatusDescriptionPair evaluateResourceStatus(String resourceName) {
// external view or went to ERROR state (which means that we tried to load the segments/resources but failed for
// some reason)
Map<String, String> partitionStateMap = getPartitionStateMap(helixState);
- for (String partitionName : idealState.getPartitionSet()) {
- String idealStateStatus = idealState.getInstanceStateMap(partitionName).get(_instanceName);
+ for (String partitionName : partitionSet) {
+ String idealStateStatus = partitionToIdealStateStatusMap.getOrDefault(partitionName,
+ idealState.getInstanceStateMap(partitionName).get(_instanceName));
Review Comment:
(nit) We can just use get() instead of getOrDefault() since the map value has been set as `idealState.getInstanceStateMap(partitionName).get(_instanceName)` on L448?
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13541:
URL: https://github.com/apache/pinot/pull/13541#discussion_r1674706317
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java:
##########
@@ -434,6 +434,30 @@ private StatusDescriptionPair evaluateResourceStatus(String resourceName) {
return new StatusDescriptionPair(Status.GOOD, STATUS_DESCRIPTION_NONE);
}
+ // Cache ideal state status to avoid reading it twice
+ Map<String, String> partitionToIdealStateStatusMap = new HashMap<>();
+ Set<String> partitionSet = idealState.getPartitionSet();
+
+ // Check that at least one partition is assigned in the ideal state, since if no partitions are assigned
+ // then consider the resource status GOOD. This check is needed to handle the case where the resource no
+ // longer exists on the server (i.e., last segment on the server was removed), but it still exists in the
+ // ideal state
+ boolean atLeastOnePartitionAssigned = false;
+ for (String partitionName : partitionSet) {
+ String idealStateStatus = idealState.getInstanceStateMap(partitionName).get(_instanceName);
Review Comment:
We can reduce the map lookup by looping over entries in `idealState.getRecord().getMapFields()`, same for the for loop below
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13541:
URL: https://github.com/apache/pinot/pull/13541#issuecomment-2208551263
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13541?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `14 lines` in your changes missing coverage. Please review.
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`714c736`)](https://app.codecov.io/gh/apache/pinot/commit/714c736679b2e4beeb4b040f824bd6987a03e4ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 715 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13541?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/pinot/common/utils/ServiceStatus.java](https://app.codecov.io/gh/apache/pinot/pull/13541?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2FServiceStatus.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VydmljZVN0YXR1cy5qYXZh) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13541?src=pr&el=tree&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 (59551e4) and HEAD (714c736). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (714c736) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13541 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2482 +46
Lines 133233 137220 +3987
Branches 20636 21301 +665
=============================================
- Hits 82274 0 -82274
- Misses 44911 137220 +92309
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13541/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/13541/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/13541/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13541/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/13541/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13541/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/13541/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13541/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13541/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13541/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13541/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/13541/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/13541/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/13541?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).
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on code in PR #13541:
URL: https://github.com/apache/pinot/pull/13541#discussion_r1671357729
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java:
##########
@@ -443,8 +467,9 @@ private StatusDescriptionPair evaluateResourceStatus(String resourceName) {
// external view or went to ERROR state (which means that we tried to load the segments/resources but failed for
// some reason)
Map<String, String> partitionStateMap = getPartitionStateMap(helixState);
- for (String partitionName : idealState.getPartitionSet()) {
- String idealStateStatus = idealState.getInstanceStateMap(partitionName).get(_instanceName);
+ for (String partitionName : partitionSet) {
+ String idealStateStatus = partitionToIdealStateStatusMap.getOrDefault(partitionName,
+ idealState.getInstanceStateMap(partitionName).get(_instanceName));
Review Comment:
I think it's needed, otherwise we'll skip checking some partition's statuses as we break when collecting ideal state statuses on L453. Added a test case to illustrate this
--
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] Fix bug when evaluating resource status during service startup check [pinot]
Posted by "itschrispeck (via GitHub)" <gi...@apache.org>.
itschrispeck commented on PR #13541:
URL: https://github.com/apache/pinot/pull/13541#issuecomment-2231869927
> I don't fully follow the change. When there is no partition assigned to the given instance, the loop should all be skipped, and it should return GOOD right?
Not sure I understand, are you referring to [this loop](https://github.com/apache/pinot/blob/cf1a0f6b9b44fdb567452b63a34e76ac5c635429/pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java#L446-L468)? If so, the if condition linked in the PR description shows that it will return `STARTING` status if the current state is null
Or if you mean the [outer loop](https://github.com/apache/pinot/blob/cf1a0f6b9b44fdb567452b63a34e76ac5c635429/pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStatus.java#L368-L387), the issue is that `_resourceIterator` collected all assigned resources when the server first started up. The resources being iterated through are not necessarily still assigned to the server, and should be re-evaluated
--
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