You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/07/12 20:04:39 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request #2810: API: Use delete instead of remove in actions for clarity
aokolnychyi opened a new pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi merged pull request #2810: API: Use delete instead of remove in actions for clarity
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2810: API: Use delete instead of remove in actions for clarity
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810#issuecomment-878681918
I'm okay with either remove or delete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2810: API: Use delete instead of remove in actions for clarity
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810#discussion_r668183222
##########
File path: api/src/main/java/org/apache/iceberg/actions/DeleteReachableFiles.java
##########
@@ -51,39 +51,39 @@
* @param executorService the service to use
* @return this for method chaining
*/
- RemoveReachableFiles executeDeleteWith(ExecutorService executorService);
+ DeleteReachableFiles executeDeleteWith(ExecutorService executorService);
/**
* Set the {@link FileIO} to be used for files removal
*
* @param io FileIO to use for files removal
* @return this for method chaining
*/
- RemoveReachableFiles io(FileIO io);
+ DeleteReachableFiles io(FileIO io);
/**
* The action result that contains a summary of the execution.
*/
interface Result {
/**
- * Returns the number of data files removed.
+ * Returns the number of deleted data files.
*/
- long removedDataFilesCount();
+ long deletedDataFilesCount();
Review comment:
Now it matches the naming in `ExpireSnapshots`.
The downside is that we will add `deletedDeleteFilesCount` in the future. I can live with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #2810: API: Use delete instead of remove in actions for clarity
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810#issuecomment-878725910
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #2810: API: Use delete instead of remove in actions for clarity
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810#issuecomment-878526367
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #2810: API: Use delete instead of remove in actions for clarity
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #2810:
URL: https://github.com/apache/iceberg/pull/2810#issuecomment-878641339
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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org