Skip to content

Core: Add snapshot table property#1795

Closed
aokolnychyi wants to merge 1 commit intomasterfrom
snapshot-flag
Closed

Core: Add snapshot table property#1795
aokolnychyi wants to merge 1 commit intomasterfrom
snapshot-flag

Conversation

@aokolnychyi
Copy link
Contributor

This PR adds a snapshot table property. When a table created as a snapshot, we are not allowed to expire snapshots as this may delete original files.

@aokolnychyi
Copy link
Contributor Author

@RussellSpitzer @rdblue this is the snapshot flag we talked about.

It is an open question whether we want to allow users to set/modify this property. Since we consider adding an import procedure to support partial migration/snapshot, we may allow setting this property by hand.


ValidationException.check(
!PropertyUtil.propertyAsBoolean(base.properties(), SNAPSHOT, SNAPSHOT_DEFAULT),
"Not allowed to expire snapshots in a snapshot table as this may remove files in the original table");
Copy link
Member

Choose a reason for hiding this comment

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

We really need better naming. This is extremely confusing :)

Copy link
Member

@RussellSpitzer RussellSpitzer Nov 20, 2020

Choose a reason for hiding this comment

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

"The target table was created using the snapshot action against another table which means it is not the exclusive owner of its data files. This means that we cannot determine if data files are actually no longer needed after running an expireSnapshots command and could unintentionally remove files needed by the original table. Unset the SNAPSHOT table property if you are sure this what you intend to do." -- Maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, let me update this.

Choose a reason for hiding this comment

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

What about thinking about this as "un-gc'able" instead of specifically a snapshot table to be more generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is a good idea, @jacques-n. We will need a good name, though. Is gc.enabled descriptive enough? Any other alternatives?

Also, we should maybe respect this property while removing orphan files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the more specific gc enabled flag. That seems like a better way to do this.

@aokolnychyi
Copy link
Contributor Author

If we make the property as something generic, we should probably allow users to set/modify this. There may be valid use cases when the expiry of snapshots should be prohibited beyond SNAPSHOT operations.

@RussellSpitzer
Copy link
Member

I am +1 on more generic and modifiable, I think if someone really wants to do something we should let them. For example say you snapshot a table, but then stop using the original table. It's valid then to unmark the "snapshot" status and expire the old data away

@aokolnychyi
Copy link
Contributor Author

I am going to close this one as I pushed to the wrong remote here. See #1796 instead.

@aokolnychyi aokolnychyi deleted the snapshot-flag branch November 20, 2020 17:37
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

Comments