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 2020/11/20 17:35:33 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request #1796: Core: Add table property to disable garbage collection
aokolnychyi opened a new pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796
This PR adds a table property to disable garbage collection.
----------------------------------------------------------------
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.
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 a change in pull request #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527867861
##########
File path: core/src/main/java/org/apache/iceberg/TableProperties.java
##########
@@ -134,4 +134,7 @@ private TableProperties() {
public static final String ENGINE_HIVE_ENABLED = "engine.hive.enabled";
public static final boolean ENGINE_HIVE_ENABLED_DEFAULT = false;
+
+ public static final String GC_ENABLED = "gc.enabled";
+ public static final boolean GC_ENABLED_DEFAULT = true;
Review comment:
I like this. We can also add a property to require the action is used instead of the table operation to ensure that we use reachability instead of incremental changes.
----------------------------------------------------------------
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.
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] jacques-n commented on a change in pull request #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527902661
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -78,6 +81,13 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();
+
+ ValidationException.check(
+ PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT),
+ "Not allowed to expire snapshots as the garbage collection is disabled for this table. " +
+ "Make sure this table is the exclusive owner of its data files before allowing garbage collection. " +
+ "If the table was created using the SNAPSHOT operation, it is not safe to expire snapshots in it since " +
+ "this may remove files in the original table.");
Review comment:
Hmm... I'm struggling a bit with someone being allowed to update this in certain cases. It seems like a very sharp edge to expose/suggest.
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527937381
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -78,6 +81,13 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();
+
+ ValidationException.check(
+ PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT),
+ "Not allowed to expire snapshots as the garbage collection is disabled for this table. " +
+ "Make sure this table is the exclusive owner of its data files before allowing garbage collection. " +
+ "If the table was created using the SNAPSHOT operation, it is not safe to expire snapshots in it since " +
+ "this may remove files in the original table.");
Review comment:
I agree that suggesting is dangerous but letting do this should be fine too.
----------------------------------------------------------------
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.
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 a change in pull request #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527876130
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -78,6 +81,13 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();
+
+ ValidationException.check(
+ PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT),
+ "Not allowed to expire snapshots as the garbage collection is disabled for this table. " +
Review comment:
nit 'the' not needed: Not allowed to expire snapshots as garbage collection is disabled for this table.
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#issuecomment-731410240
I've merged this to unblock other PRs. Thanks everyone!
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527937381
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -78,6 +81,13 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();
+
+ ValidationException.check(
+ PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT),
+ "Not allowed to expire snapshots as the garbage collection is disabled for this table. " +
+ "Make sure this table is the exclusive owner of its data files before allowing garbage collection. " +
+ "If the table was created using the SNAPSHOT operation, it is not safe to expire snapshots in it since " +
+ "this may remove files in the original table.");
Review comment:
I agree that suggesting is dangerous but letting do this should be fine.
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#issuecomment-731375657
Let me update this one to handle orphan files.
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#issuecomment-731316431
Remove orphan files makes sense as well
----------------------------------------------------------------
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.
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 edited a comment on pull request #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#issuecomment-731410240
I've merged this to unblock other PRs since there seems to be a consensus. Thanks everyone!
----------------------------------------------------------------
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.
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 a change in pull request #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527874919
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -78,6 +81,13 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();
+
+ ValidationException.check(
+ PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT),
+ "Not allowed to expire snapshots as the garbage collection is disabled for this table. " +
+ "Make sure this table is the exclusive owner of its data files before allowing garbage collection. " +
+ "If the table was created using the SNAPSHOT operation, it is not safe to expire snapshots in it since " +
+ "this may remove files in the original table.");
Review comment:
I think it's still ok to put in instructions for disabling this parameter as a workaround. Just helps a user to know how to unblock themselves just in case. Also nit: "Cannot expire snapshots" would follow our pattern for errors in other places
----------------------------------------------------------------
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.
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 a change in pull request #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#discussion_r527936112
##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -78,6 +81,13 @@ public void accept(String file) {
RemoveSnapshots(TableOperations ops) {
this.ops = ops;
this.base = ops.current();
+
+ ValidationException.check(
+ PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT),
+ "Not allowed to expire snapshots as the garbage collection is disabled for this table. " +
+ "Make sure this table is the exclusive owner of its data files before allowing garbage collection. " +
+ "If the table was created using the SNAPSHOT operation, it is not safe to expire snapshots in it since " +
+ "this may remove files in the original table.");
Review comment:
I think it is reasonable to allow people to update it. Having properties that can only be set by the library itself seems like we're asking for people to go edit files by hand when they have a need. That said, I think it would be fine to not suggest that this can be changed. If they want to track it down by going to the code and reading this check, then fine. But we can change this to "Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)"
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#issuecomment-731306943
@jacques-n @rdblue @RussellSpitzer what about removing orphan files? Seems like we should respect this property there too.
----------------------------------------------------------------
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.
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 #1796: Core: Add table property to disable garbage collection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1796:
URL: https://github.com/apache/iceberg/pull/1796#issuecomment-731386092
Alright, this one is ready for final review round.
----------------------------------------------------------------
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.
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