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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
