Skip to content

API: Use delete instead of remove in actions for clarity#2810

Merged
aokolnychyi merged 1 commit intoapache:masterfrom
aokolnychyi:rename-remove-reachable-files-action
Jul 13, 2021
Merged

API: Use delete instead of remove in actions for clarity#2810
aokolnychyi merged 1 commit intoapache:masterfrom
aokolnychyi:rename-remove-reachable-files-action

Conversation

@aokolnychyi
Copy link
Contributor

This PR switches our new actions to use delete instead of remove as I think delete makes more sense given the naming of methods in these actions.

This is NOT a breaking change as we modify only the unreleased Actions API.

* Returns the number of deleted data files.
*/
long removedDataFilesCount();
long deletedDataFilesCount();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it matches the naming in ExpireSnapshots.
The downside is that we will add deletedDeleteFilesCount in the future. I can live with that.

@aokolnychyi aokolnychyi force-pushed the rename-remove-reachable-files-action branch from d2a5a29 to c265e5b Compare July 12, 2021 19:08
@aokolnychyi
Copy link
Contributor Author

@RussellSpitzer @flyrain @karuppayya @rdblue, I took a look at the current state of our new actions and I think it would make sense to switch from RemoveXXX to DeleteXXX in names.

Let me know what you think.

@aokolnychyi aokolnychyi force-pushed the rename-remove-reachable-files-action branch from c265e5b to b72841f Compare July 12, 2021 19:17
@RussellSpitzer
Copy link
Member

My only worry here is we have two senses of delete in the API. Logical deletes and physical deletes, before the names were distinct since "delete" apis only did logical deletes while "remove" apis actually removed files from your file system. I also only worry about this on about a 2 out of 10 worry scale so i'm +1 on moving forward if @aokolnychyi feels strongly about it :)

@rdblue
Copy link
Contributor

rdblue commented Jul 13, 2021

I'm okay with either remove or delete.

@aokolnychyi
Copy link
Contributor Author

My primary motivation is the inconsistency we have in the code right now.

In three actions (including RemoveOrphanFiles), we have deleteWith and executeDeleteWith methods that configure how to do the actual removal. We also use deletedXXXCounts in ExpireSnapshots to report the number of potentially physically removed files.

Karuppayya's original implementation called the action DeleteReachableFiles and I asked to change that to use remove but I've changed my opinion after looking at the code a little bit more.

@aokolnychyi aokolnychyi merged commit 712fe66 into apache:master Jul 13, 2021
minchowang pushed a commit to minchowang/iceberg that referenced this pull request Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants