Cover the race condition for upsert compaction #12346

snleee · 2024-01-31T21:05:06Z

The current code can face the race condition when the following condition is met:

Segment upload is complete for segment replacement and segment zk metadata is changed to point the new segment.
Server is still loading the segment (e.g. if offheap upsert is enabled, segment reload can take a while)
If minion picks up the compaction task at the above step, minion will download the new segment (replaced) while it will fetch the validDocId bitmap for old segment.

To avoid this scenario, we added the extra crc check on the minion side. Minion will check if the crc from downloaded segment and crc from server api for valid doc id bitmap are the same.

Changes from the PR:

Add the new API for fetching validDocId bitmap. the response is changed to JSON to include extra information like crc.
Wired the new API & added tests
Added the crc check on the minion task executor
Added the crc check on the task generator

Release Note on the backward incompatibility

This PR is introducing backward incompatibility for UpsertCompactionTask. Previously, we allowed to configure the compaction task without the snapshot enabled. We found that using in-memory based validDocIds is a bit dangerous as it will not give us the consistency (e.g. fetching validDocIds bitmap while the server is restarting & updating validDocIds).

We now enforce the enableSnapshot=true for UpsertCompactionTask if the advanced customer wants to run the compaction with the in-memory validDocId bitmap.

{
  "upsertConfig": {
    "mode": "FULL",
    "enableSnapshot": true
  }
}
...
"task": {
  "taskTypeConfigsMap": {
    "UpsertCompactionTask": {
      "schedule": "0 */5 * ? * *",
      "bufferTimePeriod": "7d",
      "invalidRecordsThresholdPercent": "30",
      "invalidRecordsThresholdCount": "100000",
      "invalidDocIdsType": "SNAPSHOT/IN_MEMORY/IN_MEMORY_WITH_DELETE"
    }
  }
}

Also, we allow to configure invalidDocIdsType to UpsertCompactionTask for advanced user.

snapshot: Default validDocIds type. This indicates that the validDocIds bitmap is loaded from the snapshot from the Pinot segment. UpsertConfig's enableSnapshot must be enabled for this type.
onHeap: the validDocIds bitmap will be fetched from the server.
onHeapWithDelete: the validDocIds bitmap will be fetched from the server. This will also take account into the deleted documents. UpsertConfig's deleteRecordColumn must be provided for this type.

codecov-commenter · 2024-01-31T22:13:52Z

Codecov Report

Attention: 105 lines in your changes are missing coverage. Please review.

Comparison is base (3ab2834) 61.69% compared to head (cac50bd) 61.69%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
...t/controller/util/ServerSegmentMetadataReader.java	0.00%	25 Missing ⚠️
...che/pinot/server/api/resources/TablesResource.java	62.26%	14 Missing and 6 partials ⚠️
...upsertcompaction/UpsertCompactionTaskExecutor.java	0.00%	16 Missing ⚠️
...psertcompaction/UpsertCompactionTaskGenerator.java	31.81%	15 Missing ⚠️
...n/restlet/resources/ValidDocIdsBitmapResponse.java	0.00%	10 Missing ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java	11.11%	6 Missing and 2 partials ⚠️
...mmon/restlet/resources/ValidDocIdMetadataInfo.java	0.00%	5 Missing ⚠️
...inot/common/restlet/resources/ValidDocIdsType.java	0.00%	4 Missing ⚠️
...oller/api/resources/PinotTableRestletResource.java	0.00%	1 Missing ⚠️
...che/pinot/plugin/minion/tasks/MinionTaskUtils.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12346      +/-   ##
============================================
- Coverage     61.69%   61.69%   -0.01%     
  Complexity      207      207              
============================================
  Files          2422     2426       +4     
  Lines        132217   132667     +450     
  Branches      20408    20499      +91     
============================================
+ Hits          81573    81848     +275     
- Misses        44656    44820     +164     
- Partials       5988     5999      +11

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`34.94% <0.00%> (-26.71%)`	⬇️
java-21	`61.57% <28.08%> (+<0.01%)`	⬆️
skip-bytebuffers-false	`61.66% <28.08%> (-0.01%)`	⬇️
skip-bytebuffers-true	`61.56% <28.08%> (+<0.01%)`	⬆️
temurin	`61.69% <28.08%> (-0.01%)`	⬇️
unittests	`61.68% <28.08%> (-0.01%)`	⬇️
unittests1	`46.91% <0.00%> (+0.02%)`	⬆️
unittests2	`27.69% <28.08%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The current code can face the race condition when the following condition is met: 1. Segment upload is complete for segment replacement and segment zk metadata is changed to point the new segment. 2. Server is still loading the segment (e.g. if offheap upsert is enabled, segment reload can take a while) 3. If minion picks up the compaction task at the above step, minion will download the new segment (replaced) while it will fetch the validDocId bitmap for old segment. To avoid this scenario, we added the extra crc check on the minion side. Minion will check if the crc from downloaded segment and crc from server api for valid doc id bitmap are the same. Changes from the PR: 1. Add the new API for fetching validDocId bitmap. the response is changed to JSON to include extra information like crc. 2. Wired the new API & added tests 3. Added the crc check on the minion task executor 4. Added the crc check on the task generator

...t-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdMetadataInfo.java

...ommon/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java

Jackie-Jiang · 2024-01-31T22:59:09Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+            Response.Status.BAD_REQUEST);
+      }
+
+      // Adopt the same logic as the query execution to get the valid doc ids. 'FilterPlanNode.run()'


Not introduced in this PR, but this logic is incorrect. We cannot read the valid doc ids from memory because it might be modified during ingestion, and if the server re-consume the data (e.g. restart), some valid docs might be missing. We should use the valid doc ids from the snapshot. Same for the valid doc ids metadata api

added validDocIdType. Can you review the code?

snleee · 2024-02-01T10:47:22Z

...ava/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskGenerator.java

@@ -160,6 +170,20 @@ public List<PinotTaskConfig> generateTasks(List<TableConfig> tableConfigs) {
    return pinotTaskConfigs;
  }

+  private String getDefaultValidDocId(UpsertConfig upsertConfig) {


@Jackie-Jiang I found that the snapshot is only available if enableSnapshot=true.

Should we perform upsert at the best effort or do we want to enforce enableSnapshot=true?

updated

throw error if enableSnapshot is not set to true.

throw error if queryableDocId type is set while upsert delete column is not enabled.

tibrewalpratik17 · 2024-02-01T21:38:42Z

...ava/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskGenerator.java

@@ -52,6 +54,7 @@ public class UpsertCompactionTaskGenerator extends BaseTaskGenerator {
  private static final String DEFAULT_BUFFER_PERIOD = "7d";
  private static final double DEFAULT_INVALID_RECORDS_THRESHOLD_PERCENT = 0.0;
  private static final long DEFAULT_INVALID_RECORDS_THRESHOLD_COUNT = 0;
+  private static final String DEFAULT_VALID_DOC_ID_TYPE = "validDocIdsSnapshot";


can the default be just validDocIds to keep it backward-compatible?

@tibrewalpratik17 We found that using in-memory based validDocIds is a bit dangerous as it will not give us the consistency (e.g. fetching validDocIds bitmap while the server is restarting & updating validDocIds)

by default, we would want to enforce the customer to use segment compaction + snapshot enabled in the upsert config

We would still want to allow the previous approach (im-memory bitmap based). In this case, we will ask the customers to pick the appropriate validDocIdType.

How do you think?

cc: @Jackie-Jiang

In that case, can we tag this PR as backward-incompatible as existing compaction tasks will break without snapshot?

I agree of this inconsistent scenario:

in-memory based validDocIds is a bit dangerous as it will not give us the consistency (e.g. fetching validDocIds bitmap while the server is restarting & updating validDocIds)

...ava/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskGenerator.java

Jackie-Jiang

Can you please update the PR description with what is backward incompatible?

...t-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdMetadataInfo.java

...ommon/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java

snleee · 2024-02-02T04:44:13Z

@Jackie-Jiang @tibrewalpratik17 Please take a final look on this PR. I think that this is now close to be merged.

pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsType.java

...t-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdMetadataInfo.java

...ommon/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java

* Cover the race condition for upsert compaction The current code can face the race condition when the following condition is met: 1. Segment upload is complete for segment replacement and segment zk metadata is changed to point the new segment. 2. Server is still loading the segment (e.g. if offheap upsert is enabled, segment reload can take a while) 3. If minion picks up the compaction task at the above step, minion will download the new segment (replaced) while it will fetch the validDocId bitmap for old segment. To avoid this scenario, we added the extra crc check on the minion side. Minion will check if the crc from downloaded segment and crc from server api for valid doc id bitmap are the same. Changes from the PR: 1. Add the new API for fetching validDocId bitmap. the response is changed to JSON to include extra information like crc. 2. Wired the new API & added tests 3. Added the crc check on the minion task executor 4. Added the crc check on the task generator * Add the 'validDocIdType' across all APIs * Changed 'validDocIdType' to 'validDocIdsType' * Add validDocIdsType enum * Addressing the comments to use enum everywhere

snleee requested a review from Jackie-Jiang January 31, 2024 21:05

snleee force-pushed the fix-race-condition-upsert branch 5 times, most recently from 90c5517 to 35a1ed1 Compare January 31, 2024 21:34

snleee force-pushed the fix-race-condition-upsert branch from 35a1ed1 to d89e8bc Compare January 31, 2024 22:37

snleee force-pushed the fix-race-condition-upsert branch from d89e8bc to 5c5d922 Compare January 31, 2024 22:40

Jackie-Jiang added upsert ingestion bugfix labels Jan 31, 2024

Jackie-Jiang reviewed Jan 31, 2024

View reviewed changes

snleee commented Feb 1, 2024

View reviewed changes

snleee force-pushed the fix-race-condition-upsert branch 2 times, most recently from 981a4fd to 2ec8c54 Compare February 1, 2024 19:45

Add the 'validDocIdType' across all APIs

84fa0c9

snleee force-pushed the fix-race-condition-upsert branch from 2ec8c54 to 84fa0c9 Compare February 1, 2024 21:24

tibrewalpratik17 reviewed Feb 1, 2024

View reviewed changes

...ava/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskGenerator.java Show resolved Hide resolved

...ava/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskGenerator.java Outdated Show resolved Hide resolved

snleee added the backward-incompat Referenced by PRs that introduce or fix backward compat issues label Feb 1, 2024

Jackie-Jiang reviewed Feb 2, 2024

View reviewed changes

...t-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdMetadataInfo.java Outdated Show resolved Hide resolved

...ommon/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java Outdated Show resolved Hide resolved

Changed 'validDocIdType' to 'validDocIdsType'

e9d9790

snleee added the release-notes Referenced by PRs that need attention when compiling the next release notes label Feb 2, 2024

snleee force-pushed the fix-race-condition-upsert branch from fde4c34 to b8f847a Compare February 2, 2024 04:43

Add validDocIdsType enum

83eba44

snleee force-pushed the fix-race-condition-upsert branch from b8f847a to 83eba44 Compare February 2, 2024 04:46

tibrewalpratik17 approved these changes Feb 2, 2024

View reviewed changes

Jackie-Jiang approved these changes Feb 2, 2024

View reviewed changes

Addressing the comments to use enum everywhere

cac50bd

snleee force-pushed the fix-race-condition-upsert branch from beae4f3 to cac50bd Compare February 2, 2024 19:32

snleee merged commit ab525ee into apache:master Feb 2, 2024
19 checks passed

snleee deleted the fix-race-condition-upsert branch February 2, 2024 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover the race condition for upsert compaction #12346

Cover the race condition for upsert compaction #12346

snleee commented Jan 31, 2024 •

edited

Loading

codecov-commenter commented Jan 31, 2024 •

edited

Loading

Jackie-Jiang Jan 31, 2024

snleee Feb 1, 2024

snleee Feb 1, 2024 •

edited

Loading

snleee Feb 1, 2024

tibrewalpratik17 Feb 1, 2024

snleee Feb 1, 2024

tibrewalpratik17 Feb 1, 2024 •

edited

Loading

Jackie-Jiang left a comment

snleee commented Feb 2, 2024 •

edited

Loading

Cover the race condition for upsert compaction #12346

Cover the race condition for upsert compaction #12346

Conversation

snleee commented Jan 31, 2024 • edited Loading

Release Note on the backward incompatibility

codecov-commenter commented Jan 31, 2024 • edited Loading

Codecov Report

Jackie-Jiang Jan 31, 2024

Choose a reason for hiding this comment

snleee Feb 1, 2024

Choose a reason for hiding this comment

snleee Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

snleee Feb 1, 2024

Choose a reason for hiding this comment

tibrewalpratik17 Feb 1, 2024

Choose a reason for hiding this comment

snleee Feb 1, 2024

Choose a reason for hiding this comment

tibrewalpratik17 Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

snleee commented Feb 2, 2024 • edited Loading

snleee commented Jan 31, 2024 •

edited

Loading

codecov-commenter commented Jan 31, 2024 •

edited

Loading

snleee Feb 1, 2024 •

edited

Loading

tibrewalpratik17 Feb 1, 2024 •

edited

Loading

snleee commented Feb 2, 2024 •

edited

Loading