Segment retention for table #8078
Conversation
walterddr
force-pushed
the
segment_retention_for_table
branch
from
c96dae6 to
b0c5b99
Compare
Codecov Report
@@ Coverage Diff @@
## master #8078 +/- ##
=============================================
- Coverage 71.35% 30.60% -40.76%
=============================================
Files 1617 1613 -4
Lines 83896 83822 -74
Branches 12543 12562 +19
=============================================
- Hits 59866 25650 -34216
- Misses 19944 55879 +35935
+ Partials 4086 2293 -1793
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
walterddr
force-pushed
the
segment_retention_for_table
branch
from
b0c5b99 to
0b1757a
Compare
walterddr
force-pushed
the
segment_retention_for_table
branch
from
0b1757a to
188e7d6
Compare
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
This does not work if the retention from table config is not 0. the SegmentDeletionManager.removeAgedDeletedSegments() does not take the config from the table config
...pi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
Outdated
Show resolved
Hide resolved
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
Ahh. GOOD POINT! Didn't really thought of that. let me fix this. |
walterddr
force-pushed
the
segment_retention_for_table
branch
3 times, most recently
from
3f0837f to
61dfc07
Compare
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
We still need to do postprocess() to remove the orphan deleted segments. E.g. User deletes a table, the table config won't exist in the next run, so removeAgedDeletedSegments() won't be called.
There was a problem hiding this comment.
Let's just pass in the tableConfig to this method. Calling _segmentDeletionManager.getRetentionFromTableConfig(tableConfig) then passing it in is slightly confusing
There was a problem hiding this comment.
IMO passing in tableConfig will be more confusing because, it is possible to use a retentionMs that is not coming from tableConfig; and also there's no point passing in table name separately either.
Let's not restrict the API to only work on tableConfig. segmentDeletionManager only needs a zkpath and deep store path to work IMO.
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
...pi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
Outdated
Show resolved
Hide resolved
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
...troller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
Outdated
Show resolved
Hide resolved
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
...troller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
Outdated
Show resolved
Hide resolved
walterddr
force-pushed
the
segment_retention_for_table
branch
from
daa2b91 to
4b16ccb
Compare
noted that once table is deleted, we no longer honor the overwrite from table config because the table has been deleted
walterddr
force-pushed
the
segment_retention_for_table
branch
from
4b16ccb to
1832cd0
Compare
| _retentionTimeValue = retentionTimeValue; | ||
| } | ||
|
|
||
| public String getDeletedSegmentRetentionTimeUnit() { |
There was a problem hiding this comment.
Please do not introduce a time unit config here. Retention can be specified in time period like "2d" or "3h"
We can use TimeUtils.convertPeriodToMillis() to translate
There was a problem hiding this comment.
should we also change the existed retentionTimeUnit/retentionTimeValue too? I felt like we should make them consistent
There was a problem hiding this comment.
In an installation-free world, that will be a nice thing to do. But many of us are running live installations of Pinot now. So, let us not change existing configurations casually. We can migrate the existing configrations to a new method, but this i s not the PR to do it
There was a problem hiding this comment.
done. plz kindly take another look.
|
discussed with @Jackie-Jiang offline and we decided to change how we store the "retention period" overwrite by appending it to the DELETED segment filename during the move operation. will close this one and create a new PR to do so |
4 participants
Description
We would like to add a table level delete retention config that overwrites cluster level retention config.
This table-level retention before delete in deep store is honored as long as the table exists and wasn't deleted. Once table is deleted. We will return to use the cluster-level retention for removing residual segments.
see discussion in #8072 for more details.