You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "cypherean (via GitHub)" <gi...@apache.org> on 2025/11/20 17:39:01 UTC
[PR] Set skipCrcCheckOnLoad in tableConfig [pinot]
cypherean opened a new pull request, #17249:
URL: https://github.com/apache/pinot/pull/17249
Fix for #17233
This PR adds skipCrcCheckOnLoad as table config as an alternate to server level config to allow for fine grained control on disabling CRC check for tables.
New config added: `skipCrcCheckOnLoad` under segmentsConfig in table config
## Test Plan
Tested with Pinot batch quickstart:
1. With skipCrcCheckOnLoad config enabled in airlineStats table:
```
Handling message: ZnRecord=cb7bee95-dde7-41c9-a0ad-53e4a26e6381, {CREATE_TIMESTAMP=1763551164828, EXECUTE_START_TIMESTAMP=1763551164866, MSG_ID=cb7bee95-dde7-41c9-a0ad-53e4a26e6381, MSG_STATE=new, MSG_SUBTYPE=REFRESH_SEGMENT, MSG_TYPE=USER_DEFINE_MSG, PARTITION_NAME=airlineStats_OFFLINE_16074_16074_0, RESOURCE_NAME=airlineStats_OFFLINE, RETRY_COUNT=0, SRC_CLUSTER=QuickStartCluster, SRC_INSTANCE_TYPE=PARTICIPANT, SRC_NAME=Controller_127.0.0.1_9002, TGT_NAME=Server_127.0.0.1_7050, TGT_SESSION_ID=1005c2cb4d30018, TIMEOUT=-1, segmentName=airlineStats_OFFLINE_16074_16074_0, tableName=airlineStats_OFFLINE}{}{}, Stat=Stat {_version=0, _creationTime=1763551164837, _modifiedTime=1763551164837, _ephemeralOwner=0}
Replacing segment: airlineStats_OFFLINE_16074_16074_0 in table: airlineStats_OFFLINE
Replacing segment: airlineStats_OFFLINE_16074_16074_0
END: GenericClusterController.onMessage() for cluster QuickStartCluster
144 END:INVOKE CallbackHandler 26, /QuickStartCluster/INSTANCES/Broker_127.0.0.1_8000/MESSAGES listener: org.apache.helix.controller.GenericHelixController@10bdfbcc type: CALLBACK Took: 1ms
The latency of message e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7 is 38 ms
END: InstanceMessagesCache.refresh(), 2 of Messages read from ZooKeeper. took 1 ms.
Start to refresh stale message cache
END: InstanceMessagesCache.refresh(), 2 of Messages read from ZooKeeper. took 1 ms.
Start to refresh stale message cache
Waiting for lock to reload/refresh: airlineStats_OFFLINE_16074_16074_0, queue-length: 0
Acquired lock to reload/refresh segment: airlineStats_OFFLINE_16074_16074_0 (lock-time=0ms, queue-length=0)
Skipping replacing segment: airlineStats_OFFLINE_16074_16074_0 even though its CRC has changed from: 2686808396 to: 2686808519 because skipCrcCheckOnLoad is enabled
Message cb7bee95-dde7-41c9-a0ad-53e4a26e6381 completed.
Scheduling message e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7: brokerResource:airlineStats_OFFLINE, null->null
Submit task: e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7 to pool: java.util.concurrent.ThreadPoolExecutor@6350aec6[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
Message: e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7 handling task scheduled
377 END:INVOKE CallbackHandler 28, /QuickStartCluster/INSTANCES/Broker_127.0.0.1_8000/MESSAGES listener: org.apache.helix.messaging.handling.HelixTaskExecutor@702c26cd type: CALLBACK Took: 7ms
handling task: e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7 begin, at: 1763551164872
Refreshing segment: airlineStats_OFFLINE_16074_16074_0 for table: airlineStats_OFFLINE
Refreshed segment: airlineStats_OFFLINE_16074_16074_0 for table: airlineStats_OFFLINE
Message e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7 completed.
Handled request from 127.0.0.1 POST http://localhost:9002/v2/segments?tableName=airlineStats&tableType=OFFLINE, content-type multipart/form-data; charset=ISO-8859-1; boundary=httpclient_boundary_8c757373-6c11-4c17-a0c3-2d3fad879992 status code 200 OK
Delete message cb7bee95-dde7-41c9-a0ad-53e4a26e6381 from zk!
message finished: cb7bee95-dde7-41c9-a0ad-53e4a26e6381, took 11
Message: cb7bee95-dde7-41c9-a0ad-53e4a26e6381 (parent: null) handling task for airlineStats_OFFLINE:airlineStats_OFFLINE_16074_16074_0 completed at: 1763551164877, results: true. FrameworkTime: 11 ms; HandlerTime: 1 ms.
Delete message e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7 from zk!
message finished: e3a32ea1-8025-40c7-ac3f-7bfb9f2bf8a7, took 10
```
2. With skipCrcCheckOnLoad disabled on billing table by default:
```
Handling message: ZnRecord=c5bb3f12-3937-4761-99ed-b7833f02789b, {CREATE_TIMESTAMP=1763552941563, EXECUTE_START_TIMESTAMP=1763552941592, MSG_ID=c5bb3f12-3937-4761-99ed-b7833f02789b, MSG_STATE=new, MSG_SUBTYPE=REFRESH_SEGMENT, MSG_TYPE=USER_DEFINE_MSG, PARTITION_NAME=billing_OFFLINE_0, RESOURCE_NAME=billing_OFFLINE, RETRY_COUNT=0, SRC_CLUSTER=QuickStartCluster, SRC_INSTANCE_TYPE=PARTICIPANT, SRC_NAME=Controller_127.0.0.1_9002, TGT_NAME=Server_127.0.0.1_7050, TGT_SESSION_ID=1005c43051f0018, TIMEOUT=-1, segmentName=billing_OFFLINE_0, tableName=billing_OFFLINE}{}{}, Stat=Stat {_version=0, _creationTime=1763552941572, _modifiedTime=1763552941572, _ephemeralOwner=0}
CallbackHandler26, Subscribing to path: /QuickStartCluster/INSTANCES/Broker_127.0.0.1_8000/MESSAGES took: 0
START: GenericClusterController.onMessage() for cluster QuickStartCluster
Replacing segment: billing_OFFLINE_0 in table: billing_OFFLINE
Replacing segment: billing_OFFLINE_0
CallbackHandler28, Subscribing to path: /QuickStartCluster/INSTANCES/Broker_127.0.0.1_8000/MESSAGES took: 0
END: GenericClusterController.onMessage() for cluster QuickStartCluster
144 END:INVOKE CallbackHandler 26, /QuickStartCluster/INSTANCES/Broker_127.0.0.1_8000/MESSAGES listener: org.apache.helix.controller.GenericHelixController@382d549a type: CALLBACK Took: 0ms
START: InstanceMessagesCache.refresh()
The latency of message dd9dfcb1-3f9c-4c3e-b009-9ba5f24ef8bb is 30 ms
END: InstanceMessagesCache.refresh(), 2 of Messages read from ZooKeeper. took 1 ms.
Start to refresh stale message cache
Waiting for lock to reload/refresh: billing_OFFLINE_0, queue-length: 0
Acquired lock to reload/refresh segment: billing_OFFLINE_0 (lock-time=0ms, queue-length=0)
Replacing segment: billing_OFFLINE_0 because its CRC has changed from: 2262953635 to: 2262953636
Downloading and loading segment: billing_OFFLINE_0
Downloading segment: billing_OFFLINE_0 from: http://127.0.0.1:9002/segments/billing/billing_OFFLINE_0
Acquiring instance level segment download semaphore for segment: billing_OFFLINE_0, queue-length: 0
Acquired instance level segment download semaphore for segment: billing_OFFLINE_0 (lock-time=0ms, queue-length=0).
```
Also tested with realtime tables with lucene index and upsert compaction on our test clusters.
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "cypherean (via GitHub)" <gi...@apache.org>.
cypherean commented on PR #17249:
URL: https://github.com/apache/pinot/pull/17249#issuecomment-3740253274
> Since the segment management is done via a state machine, the only way to get consistent behavior is to always ignore the crc check when loading segment, which also means segment refresh (in-place replacement) won't be supported. Do you see this as an okay trade-off? If so, let's clearly document the behavior
@Jackie-Jiang this gap is already exposed as an instance level config. Going forward
1. Do we want to ensure this is only exposed on add, reload paths and we're able to detect replace even when server is down and treat it as such?
2. Or do we allow to skip crc check for all 3 as is the current behavior and document the gaps for the user?
3. Or try add/replacement on a best effort basis and serve from local copy on failure (same as reload), but actively alert and repair?
My take - We need a combination of 1 and 3, not just as a short term fix but as a fallback for reliability. Suggested fixes:
1. Detect replace even when server is down and treat it as replace not add
2. Try add/replacement on a best effort basis and serve from local copy on failure - this will be behind the flag that let's us skip crc check right now.
Pushing the changes for (2), I'll look into 1 in a follow up PR
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2578589499
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
What is the exact problem we are trying to solve? If the problem is about inconsistent text index crc, the data CRC (#17264) is a better solution
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17249:
URL: https://github.com/apache/pinot/pull/17249#issuecomment-3752098265
After #17380, do you still see this change needed?
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "cypherean (via GitHub)" <gi...@apache.org>.
cypherean commented on PR #17249:
URL: https://github.com/apache/pinot/pull/17249#issuecomment-3758588318
> After #17380, do you still see this change needed?
I think this change is still needed. We should handle the already-introduced CRC skip more gracefully without limiting in-place replacement. Making it safer before deprecating the configuration feels like the right approach.
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2581651770
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
We lost all replicas of a segment.
Set up:
2 server replica + 1 deepstore copy
Failure scenario happens when segment commit + CRC mismatch in replicas + leader servers's host replacement happens. Following is the sequence:
```
1. Segment commit in progress
2. Leader server tries to upload segment to deepstore and fails
4. Follower server builds segment but invalidates its copy due to CRC mismatch
5. Follower server tries deepstore download but fails due to missing copy
6. leader server host starts getting replaced due to regular infra activity
7. Follower segment fails to download after retries from the only peer which is unavailable
8. Follower server segment goes in error state and when leader server comes up it goes in error state.
```
While we have internal infra improvement items we are pursuing two tracks:
Short term (Faster to adopt):
Make ignore CRC config introduced in https://github.com/apache/pinot/pull/16432 to a table level config so we can selectively apply it which are unaffected of the side effect.
Long term:
Data CRC (#17264) is the ideal way and when this becomes de-facto for CRC comparison, move away from the previous config.
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17249:
URL: https://github.com/apache/pinot/pull/17249#issuecomment-3559459686
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17249?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `64.28571%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.76%. Comparing base ([`8de6fa9`](https://app.codecov.io/gh/apache/pinot/commit/8de6fa9b9a24c4949307ba78d7f3894eaf59fe04?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`80398d6`](https://app.codecov.io/gh/apache/pinot/commit/80398d658b3e96a556b33aad5d78254bd018662d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 56 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17249?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://app.codecov.io/gh/apache/pinot/pull/17249?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fdata%2Fmanager%2FBaseTableDataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | 28.57% | [0 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17249?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 (8de6fa9) and HEAD (80398d6). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (8de6fa9) | HEAD (80398d6) |
>|------|------|------|
>|java-21|4|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|5|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17249 +/- ##
============================================
- Coverage 63.20% 55.76% -7.45%
+ Complexity 1428 701 -727
============================================
Files 3117 2436 -681
Lines 184391 137123 -47268
Branches 28278 21830 -6448
============================================
- Hits 116552 76468 -40084
+ Misses 58829 54221 -4608
+ Partials 9010 6434 -2576
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17249/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/17249/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/17249/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17249/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/17249/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/17249/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/17249/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.76% <64.28%> (-7.39%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17249/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.76% <64.28%> (-7.45%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17249/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.76% <64.28%> (-7.44%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17249/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.76% <64.28%> (-0.34%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17249/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/17249?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).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2548622078
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
Can we use Enablement enum instead of plain boolean? This will allow us to check if this config is explicitly set via table config and will allow us to override the instance level config.
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2558079577
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
This method is also used for segment refresh. This change will break segment refresh
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2578227557
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
Another way to think how to resolve when to do CRC ignore is based on table type:
```
Offline tables: Always do CRC check
Realtime table:
if segment.realtime.status = DONE then skip CRC check
else if segment.realtime.status = UPLOADED then do CRC check
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2581651770
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
We lost all replicas of a segment.
Set up:
2 server replica + 1 deepstore copy
Failure scenario happens when segment commit + CRC mismatch in replicas + leader servers's host replacement happens. Following is the sequence:
```
1. Segment commit in progress
2. Leader server tries to upload segment to deepstore and fails
4. Follower server builds segment but invalidates its copy due to CRC mismatch
5. Follower server tries deepstore download but fails due to missing copy
6. leader server host starts getting replaced due to regular infra activity
7. Follower segment fails to download after retries from the only peer which is unavailable
8. Follower server segment goes in error state and when leader server comes up it goes in error state.
```
While we have internal infra improvement items we are pursuing two tracks:
Short term (Faster to adopt):
Make ignore CRC config introduced in https://github.com/apache/pinot/pull/16432 to a table level config so we can selectively apply it to tables unaffected by the side effect.
Long term:
Data CRC (#17264) is the ideal way and when this becomes de-facto for CRC comparison, move away from the previous config.
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "cypherean (via GitHub)" <gi...@apache.org>.
cypherean commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2564495836
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
The idea was to provide granular control for skipping CRC at a table level instead of a server level config. IIUC skip crc at server level would cause the same issue `!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()`.
We can remove these two configs from here(& `reloadSegment`) and only check it in the `tryLoadExistingSegment` path. That way, we ensure that replace and reload behaves as intended. What do you think?
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2566494613
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
Per the intention of this flag, I think we should honor this flag on `add`, `reload`, but not on `replace`. But this could cause another corner case: if a segment is replaced when a server is down, it will be handled as `add` and then cause stale segment.
Another way is to keep it as is, but we need mechanism to reject segment replacement as it won't work
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17249:
URL: https://github.com/apache/pinot/pull/17249#issuecomment-3623954221
Since the segment management is done via a state machine, the only way to get consistent behavior is to always ignore the crc check when loading segment, which also means segment refresh (in-place replacement) won't be supported. Do you see this as an okay trade-off? If so, let's clearly document the behavior
--
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] Set skipCrcCheckOnLoad in table config [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #17249:
URL: https://github.com/apache/pinot/pull/17249#discussion_r2578227557
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -438,10 +442,10 @@ protected void replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
_logger.info("Segment: {} has CRC: {} same as before, not replacing it", segmentName, localMetadata.getCrc());
return;
}
- if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad()) {
+ if (!_instanceDataManagerConfig.shouldCheckCRCOnSegmentLoad() || _skipCrcCheckForThisTable) {
Review Comment:
Another way to think on when to do CRC check is can be on table type:
```
Offline tables: Always do CRC check
Realtime table:
if segment.realtime.status = DONE then skip CRC check
else if segment.realtime.status = UPLOADED then do CRC check
--
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