[GitHub] [pinot] deemoliu commented on pull request #10047: Add upsert ttl configs for Pinot upsert optimizations

["deemoliu (via GitHub)" <gi...@apache.org>]

deemoliu commented on PR #10047:
URL: https://github.com/apache/pinot/pull/10047#issuecomment-1532656955

   > We need to add more controls to the snapshots. We should mark snapshot persisted by TTL differently from the regular snapshot:
   > 
   > * When adding segment, we don't want to add metadata for segment with TTL snapshot
   > * We don't want to persist regular snapshot when TTL snapshot exists
   > * Do we want to allow TTL without enabling snapshot
   > * What if user want to change the TTL value or disable TTL
   
   Thanks @Jackie-Jiang I tried to solve the above comment with the following approach, can you please review?
   (1) When adding segment, we don't want to add metadata for segment with TTL snapshot
   The current implementation doesn't add metadata for segment with TTL snapshot.
   
   (2) We don't want to persist regular snapshot when TTL snapshot exists
   ```
     public static final String VALID_DOC_IDS_SNAPSHOT_FILE_NAME = "validdocids.bitmap.snapshot";
     public static final String VALID_DOC_IDS_TTL_SNAPSHOT_FILE_NAME = "validdocids.bitmap.ttl.snapshot";
   ```
   I used different snapshot name for "snapshotOnly" and "TTLEnabled", thus it's okay to persist regular snapshot when TTL snapshot exists. Do you think we still need to add this constraint in the tableConfig?
   
   (3)  Do we want to allow TTL without enabling snapshot
   We can allow enable or disable `snapshotOnly` with TTL feature, since they use different snapshots.
   Currently if TTL is enabled, the `TTLEnabled` snapshot for old segments will automatically generated during  new segment sealing. 
   
   (4) Change the TTL value and disable TTL can be supported by re-initalized ConcurrentHashmapPartitionUpsertManager. However there need to be another PR to handle some cases.
   e.g. 
   ```
       while (_nonPersistedSegmentsQueue.peek()._endTimeMs < expiredTimestamp) {
         IndexSegment segment = _nonPersistedSegmentsQueue.poll()._segment;
         ...
   ```
   TTL value change can lead to changing to expiredTimestamp, and so we might see missing validDocsSnapshot for some segments. For now. I will add a validation to avoid updating the TTL values (user need to create new table if they need to change TTL value).
   
   
   


-- 
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