You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jadami10 (via GitHub)" <gi...@apache.org> on 2024/12/06 02:32:08 UTC
[PR] include reason why TableSegmentValidationInfo is invalid [pinot]
jadami10 opened a new pull request, #14612:
URL: https://github.com/apache/pinot/pull/14612
This is an API enhancement so servers give a reason for why time boundary advance failing. We've found with tables wtih 10k+ segments, this API will repeatedly fail, but there's no way to debug what segment failed or why.
I added a unit test to ensure parsing still works without "invalidReason".
I also tested this on a cluster by manually changing the segment CRC in the zk metadata.
Calling the API when the controller was upgraded but the server wasn't gave back "null" as the error.
```
{
"code": 412,
"error": "Table segment validation failed. error=null"
}
```
Calling the API with bot components upgraded gave back the actual error.
```
{
"code": 412,
"error": "Table segment validation failed. error=Segment <table_name>_1732579200000_1733356800000_1733450209949 is in ONLINE state, but has CRC mismatch. zk_metadata_crc=1408182336, segment_data_manager_crc=1408182337"
}
```
--
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] include reason why TableSegmentValidationInfo is invalid [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14612:
URL: https://github.com/apache/pinot/pull/14612#issuecomment-2522028690
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14612?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> 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 [(`a048e3f`)](https://app.codecov.io/gh/apache/pinot/commit/a048e3f85901db83ffcabcf80641bb98678012e4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1437 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (a048e3f). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (a048e3f) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|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 #14612 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14612/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/14612/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/14612/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14612/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/14612/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14612/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/14612/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14612/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14612/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14612/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14612/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/14612/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/14612/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/14612?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] include reason why TableSegmentValidationInfo is invalid [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #14612:
URL: https://github.com/apache/pinot/pull/14612#discussion_r1879333029
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -1256,8 +1256,9 @@ private long validateSegmentStateForTable(String offlineTableName)
TableSegmentValidationInfo tableSegmentValidationInfo =
JsonUtils.stringToObject(response, TableSegmentValidationInfo.class);
if (!tableSegmentValidationInfo.isValid()) {
- throw new ControllerApplicationException(LOGGER, "Table segment validation failed",
- Response.Status.PRECONDITION_FAILED);
+ String error = String.format("Table segment validation failed. error=%s",
Review Comment:
what's the reasoning?
--
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] include reason why TableSegmentValidationInfo is invalid [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #14612:
URL: https://github.com/apache/pinot/pull/14612
--
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] include reason why TableSegmentValidationInfo is invalid [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14612:
URL: https://github.com/apache/pinot/pull/14612#discussion_r1880779926
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -1256,8 +1256,9 @@ private long validateSegmentStateForTable(String offlineTableName)
TableSegmentValidationInfo tableSegmentValidationInfo =
JsonUtils.stringToObject(response, TableSegmentValidationInfo.class);
if (!tableSegmentValidationInfo.isValid()) {
- throw new ControllerApplicationException(LOGGER, "Table segment validation failed",
- Response.Status.PRECONDITION_FAILED);
+ String error = String.format("Table segment validation failed. error=%s",
Review Comment:
More context can be found in #14404
--
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] include reason why TableSegmentValidationInfo is invalid [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14612:
URL: https://github.com/apache/pinot/pull/14612#discussion_r1878946618
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/TableSegmentValidationInfo.java:
##########
@@ -41,4 +44,8 @@ public boolean isValid() {
public long getMaxEndTimeMs() {
return _maxEndTimeMs;
}
+
+ public String getInvalidReason() {
Review Comment:
(minor) Annotate it as `@Nullable`
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/TableSegmentValidationInfo.java:
##########
@@ -25,12 +25,15 @@
public class TableSegmentValidationInfo {
private final boolean _valid;
+ private final String _invalidReason;
private final long _maxEndTimeMs;
@JsonCreator
public TableSegmentValidationInfo(@JsonProperty("valid") boolean valid,
+ @JsonProperty("invalidReason") String invalidReason,
Review Comment:
(minor) Annotate it as `@Nullable`
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -1033,7 +1033,10 @@ public TableSegmentValidationInfo validateTableSegmentState(
case SegmentStateModel.CONSUMING:
// Only validate presence of segment
if (segmentDataManager == null) {
- return new TableSegmentValidationInfo(false, -1);
+ String invalidReason = String.format(
+ "Segment %s is in CONSUMING state, but segmentDataManager is null", segmentName);
+ LOGGER.error(invalidReason);
Review Comment:
Let's not log error here. This is normal case, and returning the invalid reason through response should be good enough. Same for other places
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -1256,8 +1256,9 @@ private long validateSegmentStateForTable(String offlineTableName)
TableSegmentValidationInfo tableSegmentValidationInfo =
JsonUtils.stringToObject(response, TableSegmentValidationInfo.class);
if (!tableSegmentValidationInfo.isValid()) {
- throw new ControllerApplicationException(LOGGER, "Table segment validation failed",
- Response.Status.PRECONDITION_FAILED);
+ String error = String.format("Table segment validation failed. error=%s",
Review Comment:
(minor) We have started an effort to avoid using `String.format()` and replace it with regular concatenation. Same for other places
--
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