Add UpdateSnapshotReference #3612
Conversation
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
Thank you for working on this! I added some comments first on the API part.
It seems to me that we are missing at least the 2 methods below:
- tag(String name, long snapshotId)
- setBranch(String name, long snapshotId)
otherwise there is no way to create a branch/tag without custom config.
| /** | ||
| * API for updating snapshot reference. | ||
| * <p> | ||
| * Apply returns the updated snapshot reference as a {@link SnapshotReference} for validation. |
There was a problem hiding this comment.
it is unclear to me what you mean by this, I think this sentence is not needed because the return type is self-explanatory.
| * @return this | ||
| * @throws IllegalArgumentException If there is no such snapshot reference named name | ||
| */ | ||
| UpdateSnapshotReference removeReference(String name); |
There was a problem hiding this comment.
nit: prefer to use ref over reference, such as removeRef
| * @return this | ||
| */ | ||
|
|
||
| UpdateSnapshotReference setMaxRefAgeMs(String name, Long maxRefAgeMs); |
| * @param name new name for snapshot reference | ||
| * @return this | ||
| */ | ||
| UpdateSnapshotReference updateName(String oldName, String name); |
There was a problem hiding this comment.
I think this one and updateReference below are not needed, what are their use cases? We can always remove the old one and add a new one
There was a problem hiding this comment.
updateName is used when user want just update the name of one reference and inherit old config like reflifetime. And updateReference is used when user want update multiple properties of one old reference, so they can update these by just once operation.
| * <p> | ||
| * Apply returns the updated snapshot reference as a {@link SnapshotReference} for validation. | ||
| * <p> | ||
| * When committing, these changes will be applied to the current table metadata. Commit conflicts |
There was a problem hiding this comment.
Consider the situation that user A sets a retention policy for branch, and B sets retention policy for the same branch. Then both commits would succeed and one of the commit would be clobbered.
So I think the safest way to go is to not resolve commit conflicts. If the metadata is no longer current, a CommitFailedException should be thrown.
| * @param name name of snapshot reference what will be update | ||
| * @return this | ||
| */ | ||
|
|
| * @param name name of snapshot reference what will be update | ||
| * @return this | ||
| */ | ||
|
|
| UpdateSnapshotReference removeReference(String name); | ||
|
|
||
| /** | ||
| * Update minSnapshotsToKeep of snapshotReference what will be search by referenceName and snapshotReferenceType. |
There was a problem hiding this comment.
nit: I think we can be more concise here, something like "Set retention policy of snapshots in a branch". The retention policy settings should be explained in @param part using the definition provided in the spec (#3425)
| UpdateSnapshotReference setBranchRetention(String name, Long ageMs, Integer numToKeep); | ||
|
|
||
| /** | ||
| * Update minSnapshotsToKeep of snapshotReference what will be search by referenceName and snapshotReferenceType. |
| * @return this | ||
| */ | ||
|
|
||
| UpdateSnapshotReference setBranchRetention(String name, Long ageMs, Integer numToKeep); |
There was a problem hiding this comment.
I think we should use primitive values so that people must specify a value. Having a method call like setBranchRetention("test", null, null) is unfriendly to readers of the code.
But I understand we want to have the flexibility for users to use defaults. We can potentially introduce 2 different methods for these two settings separately, like setBranchSnapshotLifetime(String name, long ageMs) and setMinSnapshotsInBranch(String name, int num), but maybe its a bit overkill.
There was a problem hiding this comment.
I used code several method like setBranchSnapshotLifetime and setMinSnapshotsInBranch. But i also think its a bit overkill, if it's necessary i will add these method.
|
I'd like to also invite @rdblue to take a look since we have been discussing about this feature. |
|
@jackye1995 fixed all |
2 participants
Base on #3104.