Skip to content

Rename Iceberg delete_orphan_files to remove_orphan_files#12468

Merged
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/iceberg_rename_delete_orphan_files_to_remove_orphan_files
May 20, 2022
Merged

Rename Iceberg delete_orphan_files to remove_orphan_files#12468
findepi merged 2 commits intotrinodb:masterfrom
homar:homar/iceberg_rename_delete_orphan_files_to_remove_orphan_files

Conversation

@homar
Copy link
Member

@homar homar commented May 18, 2022

Description

REMOVE_ORPHAN_FILES is a name used by iceberg and spark

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.
(x) Release notes entries required with the following suggested text:
Renamed Iceberg DELETE_ORPHAN_FILES procedure to REMOVE_ORPHAN_FILES

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

@cla-bot cla-bot bot added the cla-signed label May 18, 2022
@homar homar requested review from findepi and mosabua and removed request for findepi May 18, 2022 20:46
@findepi findepi requested review from alexjo2144 and findinpath May 19, 2022 07:45
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

This is a breaking change, but we have an unintentional inconsistency with Spark procedures in what was intended to be modeled after them.

@findepi findepi requested review from electrum, hashhar, losipiuk and phd3 May 19, 2022 07:46
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Unfortunate but it's better to do such changes sooner rather than later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the current commit:
higher than -> higher than or equal to

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing this, but keep separate commit

Copy link
Member

Choose a reason for hiding this comment

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

a rename should be as mechanical as possible, and other fixes/improvements should be separate

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Let's use the lowercased convention for procedure names (as documented in iceberg.rst) REMOVE_ORPHAN_FILES -> remove_orphan_files

Copy link
Member Author

Choose a reason for hiding this comment

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

We do use VACUUM, OPTIMIZE, EXPIRE_SNAPSHOTS so I think REMOVE_ORPHAN_FIILES is perfectly fine here

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, but orthogonal

@homar homar force-pushed the homar/iceberg_rename_delete_orphan_files_to_remove_orphan_files branch from 8e0d504 to 7f6212b Compare May 19, 2022 10:20
@homar homar force-pushed the homar/iceberg_rename_delete_orphan_files_to_remove_orphan_files branch from 7f6212b to c1d894b Compare May 19, 2022 10:52
@phd3
Copy link
Member

phd3 commented May 19, 2022

LGTM, worth calling out the config property naming change in RN.

posted on slack, hopefully if anyone is seriously impacted we'll know if fallback is needed

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.

Doc changes look good. My one question would be if we should keep the old name as legacy config so this is not a breaking change..

@findepi
Copy link
Member

findepi commented May 19, 2022

My one question would be if we should keep the old name as legacy config so this is not a breaking change..

The procedure name changes.
While we could keep config name with @LegacyConfig, the damage will be done anyway. I think it's maybe better to break config name and procedure name alike.

@mosabua
Copy link
Member

mosabua commented May 19, 2022

My one question would be if we should keep the old name as legacy config so this is not a breaking change..

The procedure name changes. While we could keep config name with @LegacyConfig, the damage will be done anyway. I think it's maybe better to break config name and procedure name alike.

Fair enough ... probably need to mention it as breaking change in the release notes..

@findepi findepi merged commit acf3c9c into trinodb:master May 20, 2022
@findepi
Copy link
Member

findepi commented May 20, 2022

Thanks everyone for the involvement.
@mosabua i am not adding RN for this, i think you'll do the wording better here. Thanks

@github-actions github-actions bot added this to the 382 milestone May 20, 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.

6 participants