add retention period to deleted segment files and allow table level o… #8176

walterddr · 2022-02-09T05:27:53Z

this is a follow up with several previous attempt to fix segment file deletion.

Goal

Add support to control segment retention period on a per-table basis.

Challenge

in previous Approaches - we previously attempt to add field in TableConfig that controls the retention period. this causes many corner case problems

when table is deleted, so is the tableConfig, thus we need to store the retention period override somewhere.
when retention period changed for a table config (e.g. say from 3 days to 7 days) this means we need to rescanned all the previously deleted segments based on the new retention, however files that are older than 3 days but newer than 7 days would've already been deleted.

Therefore it is semantically hard to do a post-deletion retention override.

New Approach

In this PR we avoid these issues entirely by employing one practice: segments retentions are set when they are being deleted based on the tableConfig override or the default cluster setting AT THE TIME OF DELETION. no retro editing of the override will be supported for any deleted segments.

This means if there's any new changes to the cluster level or table level override, they will only apply to newly deleted segments and will not affect any segments that are previously deleted.

See: #8078 and #8069 for more details.

codecov-commenter · 2022-02-09T16:49:15Z

Codecov Report

Merging #8176 (5540a33) into master (df39bda) will decrease coverage by 7.17%.
The diff coverage is 70.83%.

@@             Coverage Diff              @@
##             master    #8176      +/-   ##
============================================
- Coverage     71.01%   63.84%   -7.18%     
+ Complexity     4314     4233      -81     
============================================
  Files          1624     1614      -10     
  Lines         84873    84681     -192     
  Branches      12791    12757      -34     
============================================
- Hits          60273    54063    -6210     
- Misses        20453    26673    +6220     
+ Partials       4147     3945     -202

Flag	Coverage Δ
integration1	`28.78% <60.41%> (+0.14%)`	⬆️
integration2	`27.56% <27.08%> (+0.24%)`	⬆️
unittests1	`67.33% <100.00%> (-0.12%)`	⬇️
unittests2	`?`

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

Impacted Files	Coverage Δ
.../controller/helix/core/SegmentDeletionManager.java	`58.49% <63.15%> (-14.24%)`	⬇️
...ntroller/helix/core/PinotHelixResourceManager.java	`42.55% <100.00%> (-23.44%)`	⬇️
...troller/helix/core/retention/RetentionManager.java	`26.61% <100.00%> (-56.73%)`	⬇️
...ig/table/SegmentsValidationAndRetentionConfig.java	`96.07% <100.00%> (+0.24%)`	⬆️
...he/pinot/spi/utils/builder/TableConfigBuilder.java	`82.87% <100.00%> (+0.23%)`	⬆️
...pinot/controller/recommender/io/ConfigManager.java	`0.00% <0.00%> (-100.00%)`	⬇️
...troller/recommender/io/metadata/FieldMetadata.java	`0.00% <0.00%> (-100.00%)`	⬇️
...roller/recommender/rules/impl/BloomFilterRule.java	`0.00% <0.00%> (-100.00%)`	⬇️
...oller/api/resources/PinotControllerAppConfigs.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ler/recommender/data/generator/BytesGenerator.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 252 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df39bda...5540a33. Read the comment docs.

mcvsubbu · 2022-02-09T17:14:48Z

This PR is complicating things and trying to solve a problem that may never exist.

It is fair to NOT expect that deletion of a table behave as per table config settings. it is ok to use the cluster behavior if the table config is not found. If it is desired that the segments be re-loaded onto another table, the operator should save the segments before creating a new table.

Also, I don't understand the need to re-scan all segments when table config retention time is changed.

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

walterddr · 2022-02-09T17:45:53Z

This PR is complicating things and trying to solve a problem that may never exist.

yes this requirement does exist. what we want to control is the "undo" period for a table after deletion.

It is fair to NOT expect that deletion of a table behave as per table config settings. it is ok to use the cluster behavior if the table config is not found.

Sorry let me re-clarify: the issue is that when the deletion retention period changes (whether cluster level or table level), does that retroactively apply to segments that has already been deleted. think of this sequence:

a segment is deleted at timestamp now() - 2d, at the time the retention setting was 7d
cluster retention config changed to 1d at timestamp now() - 1d.

now question: does this apply to the segment that was deleted at now() - 2d ? if so, then the segment will be immediately deleted, otherwise we should still honor the retention setting 7d at the time this segment was deleted.

If it is desired that the segments be re-loaded onto another table, the operator should save the segments before creating a new table.

reloading on to another table is not the problem I was trying to address.

Also, I don't understand the need to re-scan all segments when table config retention time is changed.

this was echo back to the example i said above: if we decided to retroactively apply the new retention period to previously deleted segments, we then need to read out all the DELETED_SEGMENTS, find their table-level override and the cluster level config. apply the retention deletion rule if they had already gone pass the new retention time.

The approach this PR propose is, we DO NOT retroactively apply changes to already-deleted segments. e.g. we always apply the config at the time of deletion - by directly encoding the retention duration into the deleted segment filename.

Jackie-Jiang

LGTM otherwise

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java

...pi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java

mcvsubbu

Will the file name encoded extension be there even if the table config does not have any override? Can we avoid that, please? So, cluster-level changes are applied immediately, but table level changes are honored at the time of deletion of the segment.

Also, for readability, you may wan to call one as "remove" and the other as "delete" (and define these in comments in SegmentDeletionManager and name the variables appropriately). Otherwise, it gets confusing.

Lastly, have you considered the possibility of file explosion? There will now be multiple copies of the same segment with different deletion dates. Is that desirable? Can it be used for DDOS?

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java

walterddr · 2022-02-10T21:51:43Z

Will the file name encoded extension be there even if the table config does not have any override? Can we avoid that, please? So, cluster-level changes are applied immediately, but table level changes are honored at the time of deletion of the segment.

yes. actually this is a good idea. let me do that.

Also, for readability, you may wan to call one as "remove" and the other as "delete" (and define these in comments in SegmentDeletionManager and name the variables appropriately). Otherwise, it gets confusing.

sound good. will do.

Lastly, have you considered the possibility of file explosion? There will now be multiple copies of the same segment with different deletion dates. Is that desirable? Can it be used for DDOS?

it won't be. once the segments are deleted they cannot be deleted once again. there's only one copied per deleted segment.

walterddr · 2022-02-11T19:19:14Z

Echo on previous comments

So, cluster-level changes are applied immediately, but table level changes are honored at the time of deletion of the segment.

this cannot be done unless controller is restarted as the retention time, so I decided to keep the time-to-delete suffix in all deleted segments.

for other comments I've addressed. plz kindly take another look

mcvsubbu · 2022-02-11T19:51:11Z

Echo on previous comments

So, cluster-level changes are applied immediately, but table level changes are honored at the time of deletion of the segment.

this cannot be done unless controller is restarted as the retention time, so I decided to keep the time-to-delete suffix in all deleted segments.

for other comments I've addressed. plz kindly take another look

It is OK to reqiure a controller restart if a cluster level change is applied. In future (another PR) we can move this config to Helix, and a restart will not be required.

Please apply this change.

walterddr · 2022-02-11T20:40:54Z

It is OK to reqiure a controller restart if a cluster level change is applied. In future (another PR) we can move this config to Helix, and a restart will not be required.

Please apply this change.

sounds good. done. will update pinot-docs accordingly once this rolls out

…verride

…thout suffix

…pplies immediately

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java

Jackie-Jiang

Let's add a test for table config override, a test for no table config override, and ensure the file names are expected

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java

...er/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java

walterddr · 2022-02-17T23:06:10Z

Let's add a test for table config override, a test for no table config override, and ensure the file names are expected

this is already added. but via mock(TableConfig.class) instead of a real table config object.

Jackie-Jiang

LGTM otherwise

Jackie-Jiang · 2022-02-18T00:04:21Z

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java

+    return lastModifiedTime + _defaultDeletedSegmentsRetentionMs;
+  }
+
+  private static Long getRetentionMsFromTableConfig(@Nullable TableConfig tableConfig) {


(minor) Annotate the return as @Nullable

Jackie-Jiang · 2022-02-18T00:07:05Z

...ler/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java

@@ -197,6 +200,15 @@ public Object answer(InvocationOnMock invocationOnMock)
        return null;
      }
    }).when(resourceManager).deleteSegments(anyString(), ArgumentMatchers.anyList());
+
+    // fake segment lineage.
+    SegmentLineage segmentLineage = new SegmentLineage(REALTIME_TABLE_NAME);


Why do we need to mock a segment lineage? Is this change related?

good catch not anymore since we fake the segment deletion manager. forgot to clean up

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java

…ix/core/SegmentDeletionManager.java Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

apache#8176) Add support to control segment retention period on a per-table basis.

walterddr force-pushed the segment_retention_to_deleted_segment_file branch from 9926115 to c62d438 Compare February 9, 2022 15:46

mcvsubbu reviewed Feb 9, 2022

View reviewed changes

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated Show resolved Hide resolved

walterddr force-pushed the segment_retention_to_deleted_segment_file branch 2 times, most recently from 4537eec to f6243a2 Compare February 10, 2022 05:37

Jackie-Jiang reviewed Feb 10, 2022

View reviewed changes

mcvsubbu reviewed Feb 10, 2022

View reviewed changes

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java Outdated Show resolved Hide resolved

walterddr force-pushed the segment_retention_to_deleted_segment_file branch 2 times, most recently from 715cb13 to 77f5e21 Compare February 11, 2022 17:10

Rong Rong added 5 commits February 14, 2022 20:03

add retention period to deleted segment files and allow table level o…

5295af9

…verride

address diff comments

fd182c6

modify the storage format to readable simple date

e986f9b

also add segmet deletion manager test for default retention period wi…

046875a

…thout suffix

do not encode default cluster deletion so that cluster level change a…

74dabbe

…pplies immediately

walterddr force-pushed the segment_retention_to_deleted_segment_file branch from 93ba495 to 569206a Compare February 15, 2022 04:03

make retention manager take tableConfig instead of retention time

8433b3b

walterddr force-pushed the segment_retention_to_deleted_segment_file branch from 569206a to 8433b3b Compare February 15, 2022 16:22

mcvsubbu requested changes Feb 15, 2022

View reviewed changes

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

walterddr force-pushed the segment_retention_to_deleted_segment_file branch 2 times, most recently from ac3fc2a to 90499e1 Compare February 15, 2022 18:37

address diff comments

62e4ab5

walterddr force-pushed the segment_retention_to_deleted_segment_file branch from 90499e1 to 62e4ab5 Compare February 15, 2022 19:32

mcvsubbu approved these changes Feb 17, 2022

View reviewed changes

walterddr mentioned this pull request Feb 17, 2022

adding instant deletion option for segment deletion #8122

Merged

Jackie-Jiang reviewed Feb 17, 2022

View reviewed changes

address diff comments

ab894d6

Jackie-Jiang approved these changes Feb 18, 2022

View reviewed changes

fix minor

dd04096

walterddr force-pushed the segment_retention_to_deleted_segment_file branch from 5ac97fa to dd04096 Compare February 18, 2022 00:39

Jackie-Jiang reviewed Feb 18, 2022

View reviewed changes

...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java Outdated Show resolved Hide resolved

Update pinot-controller/src/main/java/org/apache/pinot/controller/hel…

5540a33

…ix/core/SegmentDeletionManager.java Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

Jackie-Jiang merged commit 287f552 into apache:master Feb 18, 2022

xiangfu0 pushed a commit to xiangfu0/pinot that referenced this pull request Feb 23, 2022

add retention period to deleted segment files and allow table level o… (

1a6a7d7

apache#8176) Add support to control segment retention period on a per-table basis.

walterddr deleted the segment_retention_to_deleted_segment_file branch December 6, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add retention period to deleted segment files and allow table level o… #8176

add retention period to deleted segment files and allow table level o… #8176

walterddr commented Feb 9, 2022

codecov-commenter commented Feb 9, 2022 •

edited

Loading

mcvsubbu commented Feb 9, 2022

walterddr commented Feb 9, 2022 •

edited

Loading

Jackie-Jiang left a comment

mcvsubbu left a comment

walterddr commented Feb 10, 2022

walterddr commented Feb 11, 2022 •

edited

Loading

mcvsubbu commented Feb 11, 2022

walterddr commented Feb 11, 2022

Jackie-Jiang left a comment

walterddr commented Feb 17, 2022

Jackie-Jiang left a comment

Jackie-Jiang Feb 18, 2022

Jackie-Jiang Feb 18, 2022

walterddr Feb 18, 2022

add retention period to deleted segment files and allow table level o… #8176

add retention period to deleted segment files and allow table level o… #8176

Conversation

walterddr commented Feb 9, 2022

Goal

Challenge

New Approach

codecov-commenter commented Feb 9, 2022 • edited Loading

Codecov Report

mcvsubbu commented Feb 9, 2022

walterddr commented Feb 9, 2022 • edited Loading

Jackie-Jiang left a comment

Choose a reason for hiding this comment

mcvsubbu left a comment

Choose a reason for hiding this comment

walterddr commented Feb 10, 2022

walterddr commented Feb 11, 2022 • edited Loading

mcvsubbu commented Feb 11, 2022

walterddr commented Feb 11, 2022

Jackie-Jiang left a comment

Choose a reason for hiding this comment

walterddr commented Feb 17, 2022

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Feb 18, 2022

Choose a reason for hiding this comment

Jackie-Jiang Feb 18, 2022

Choose a reason for hiding this comment

walterddr Feb 18, 2022

Choose a reason for hiding this comment

codecov-commenter commented Feb 9, 2022 •

edited

Loading

walterddr commented Feb 9, 2022 •

edited

Loading

walterddr commented Feb 11, 2022 •

edited

Loading