Use a single map to store and update the previous Record location keys when reverting Upsert Metadata

[deepthi912]

Much better way of reverting keys is having single source of truth map to revert the primary keys in Upserts and have a single abstraction than distributing the keys to two different maps. This helps us keep the code error prone and easy to understand.

Modified the server config: pinot.server.consuming.segment.consistency.mode to take in three modes for force commit/Reload on Partial Upsert Tables and Upsert Tables with dropOutOfOrder.

The three modes are as follows:

1. DEFAULT : Reloads and force commits are disabled for Partial Upsert Tables and Upsert Tables with dropOutOfOrder=true and consistencyMode=NONE with RF>1. This mode is enabled by default.
2. PROTECTED : Reloads, safer commits and removals are enabled, and the upsert metadata is reverted to eliminate in case of any inconsistencies during consuming segment replacement or consuming segment removal. Safer way of handling the metadata.
3. UNSAFE : Reload is enabled but the upsert metadata is not reverted during consuming segment's replacement. This mode is very unsafe

Note:

• If the previous location segment no longer exists or the segment is deleted, then the pks will be deleted in the metadata map and the next entries for those pks are considered new. In short, currently there is no way to revert and merge in partial upserts during segment deletions. Consistency might not be garunteed during segment deletions.
• During Ingestion, always make sure the ParallelSegmentConsumptionPolicy is DISALLOW_ALWAYS or ALLOW_DURING_BUILD_ONLY for the Partial Upsert and Upsert Tables with dropOutOfOrder=true when consistencyMode = NONE to avoid any inconsistencies in PROTECTED mode. Otherwise, we might see inconsistent records across servers if at any given point we have two segments in Mutable state. Refer: Allows consumption during build for dedup/partial-upsert #15296

AddRecord:

• comparison value < 0 : NO_OP
• comparison value >=0
  • Same Segment : NO_OP
  • Different segment :
    • Add to _previousKeyToRecordLocationMap, if Immutable segment
• New Key : NO_OP

Replace Segment:

• comparison value < 0
  • current Location : Consuming Segment
    • Look up _previousKeyToRecordLocationMap, update if >= prev RecordLocation
    • Look up _previousKeyToRecordLocationMap, if not exists add to map
• comparison value >=0
  • Current Location: Consuming Segment, Remove from _previousKeyToRecordLocationMap
  • Current Location: Immutable Segment, NO_OP
• New Key: NO_OP

After Replacement: In Revert and Remove Segment

• Consuming Segment Replaced & PROTECTED_RELOAD = true & (partial Upsert || dropOutOfOrder)
  • For remaining valid Docs, lookup map:
    • Still existing location to the current consuming segment, segment is not in deleted segments and previous location exist:
      • Revert the docId to previous location and update upsert metadata map.
      • Remove entry from _previousKeyToRecordLocationMap
    • If not exist remove key from Upsert Metadata and _previousKeyToRecordLocationMap
• Current location in Immutable Segment: NO_OP

Remove Segment:

• Current Location in Consuming Segment: Revert the metadata if (partial Upsert || dropOutOfOrder)
• Current location in Immutable Segment: NO_OP

Improvisation for: #17324

Success Criteria:

Consuming Segment Reload

Expected behaviour: Consuming segment reload will force commit consuming segment, so no in consistencies should be seen between the replicas.

Force Commit

Expected behaviour: Force commit when PROTECTED is enabled should not lead to any inconsistencies between replicas.

Consuming segment removal

Expected behaviour: All the metadata of the consuming segment should be either reverted to previous location or new keys should be removed completely from the upsert metadata when PROTECTED mode is enabled.

ParallelSegmentConsumptionPolicy : ALLOW_ALWAYS(true, true), ALLOW_DURING_BUILD_ONLY(true, false), ALLOW_DURING_DOWNLOAD_ONLY(false, true), DISALLOW_ALWAYS

Expected behaviour: DISALLOW_ALWAYS and ALLOW_DURING_BUILD_ONLY should have no discrepencies. In other scenarios, we need to have metrics and alerts generated in case of inconsistencies

Try out on 3 modes of consuming segment commit protection: NONE, PROTECTED, UNSAFE for the property: pinot.server.consuming.segment.consistency.mode

[codecov-commenter]

Codecov Report

❌ Patch coverage is 26.98413% with 184 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.16%. Comparing base (36ac509) to head (d42de1d).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...tionUpsertMetadataManagerForConsistentDeletes.java	25.00%	64 Missing and 5 partials ⚠️
...t/ConcurrentMapPartitionUpsertMetadataManager.java	22.03%	43 Missing and 3 partials ⚠️
...cal/upsert/BasePartitionUpsertMetadataManager.java	33.33%	14 Missing and 6 partials ⚠️
...utils/ConsumingSegmentConsistencyModeListener.java	50.00%	16 Missing and 1 partial ⚠️
...apache/pinot/segment/local/upsert/UpsertUtils.java	0.00%	10 Missing ⚠️
...server/starter/helix/HelixInstanceDataManager.java	0.00%	8 Missing ⚠️
.../pinot/core/data/manager/BaseTableDataManager.java	0.00%	5 Missing ⚠️
.../core/realtime/PinotLLCRealtimeSegmentManager.java	40.00%	2 Missing and 1 partial ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java	0.00%	2 Missing ⚠️
.../pinot/server/starter/helix/BaseServerStarter.java	0.00%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #17503      +/-   ##
============================================
- Coverage     63.18%   63.16%   -0.03%     
  Complexity     1479     1479              
============================================
  Files          3173     3173              
  Lines        189917   189968      +51     
  Branches      29064    29084      +20     
============================================
- Hits         120002   119996       -6     
- Misses        60575    60640      +65     
+ Partials       9340     9332       -8     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-11	63.14% <26.98%> (-0.03%)	⬇️
java-21	55.47% <0.42%> (-7.66%)	⬇️
temurin	63.16% <26.98%> (-0.03%)	⬇️
unittests	63.16% <26.98%> (-0.03%)	⬇️
unittests1	55.53% <0.42%> (+0.03%)	⬆️
unittests2	34.02% <26.98%> (-0.05%)	⬇️

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.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.