Skip to content

Add docs for iceberg expire snapshots and delete orphan files#12093

Merged
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/add_docs_for_iceberg_expire_snapshots_and_delete_orphan_files
Apr 28, 2022
Merged

Add docs for iceberg expire snapshots and delete orphan files#12093
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/add_docs_for_iceberg_expire_snapshots_and_delete_orphan_files

Conversation

@homar
Copy link
Member

@homar homar commented Apr 22, 2022

Description

fixes #12086

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 22, 2022
@github-actions github-actions bot added the docs label Apr 22, 2022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase

@homar homar requested a review from mosabua April 22, 2022 08:55
@homar homar force-pushed the homar/add_docs_for_iceberg_expire_snapshots_and_delete_orphan_files branch from c2d7e74 to 382c1b8 Compare April 22, 2022 09:19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The procedure affects all snapshots that are older than the time period configured with the retention_threshold parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop "config" or use "catalog configuration property" ..

also probably reword the sentence to be specific .. "safety measure" doesnt really spell out what happens .. I think this is the case... snapshots within min-retention time period are not affected by the procedure.. its basically an override ..

example scenario: min-retention set to 30 days

call with retention_threshold 21 days
snapshots from 30 days ago and older are deleted ..

call with 30 days
nothing happens

call with 60 days
snapshots from 60 days ago and older are deleted ..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, you just can't run the procedure with retention_threshold smaller than iceberg.expire_snapshots.min-retention

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok .. well.. then we need to clarify that in the docs.

And also .. is there an useful error message when you try?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes there is an useful message but it is quite long: Retention specified (33.00s) is shorter than the minimum retention configured in the system (7.00d). Minimum retention can be changed with iceberg.expire_snapshots.min-retention configuration property or iceberg.expire_snapshots_min_retention session property

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove space before "The"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar feedback to above

@homar homar force-pushed the homar/add_docs_for_iceberg_expire_snapshots_and_delete_orphan_files branch from 382c1b8 to f4f01e3 Compare April 25, 2022 08:49
@findepi findepi requested a review from mosabua April 25, 2022 11:11
@homar homar force-pushed the homar/add_docs_for_iceberg_expire_snapshots_and_delete_orphan_files branch from 351f417 to f4f01e3 Compare April 25, 2022 11:43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is too convoluted . how about

The value for ``retention_threshold`` must be higher than ``iceberg.expire_snapshots.min-retention`` in the catalog.

@homar homar force-pushed the homar/add_docs_for_iceberg_expire_snapshots_and_delete_orphan_files branch from f4f01e3 to f4a3673 Compare April 26, 2022 08:24
@homar
Copy link
Member Author

homar commented Apr 27, 2022

@mosabua are we good to go with this ?

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we wrap the source at 80 char but its more important to get this merged ..

@mosabua
Copy link
Member

mosabua commented Apr 27, 2022

@martint could we get this in for 379 .. feature shipped in 378 already..

@findepi findepi merged commit 57983d9 into trinodb:master Apr 28, 2022
@github-actions github-actions bot added this to the 379 milestone Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Document expire_snapshots and delete_orphan_files table procedures

3 participants