jackye1995 commented on a change in pull request #3883: URL: https://github.com/apache/iceberg/pull/3883#discussion_r791218239
########## File path: core/src/main/java/org/apache/iceberg/BaseUpdateSnapshotRefs.java ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iceberg; + +import java.util.Map; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; + +public class BaseUpdateSnapshotRefs implements UpdateSnapshotRefs { + + private final TableOperations ops; + private final TableMetadata base; + private final Map<String, SnapshotRef> refs; + + BaseUpdateSnapshotRefs(TableOperations ops) { + this.ops = ops; + this.base = ops.current(); + this.refs = Maps.newHashMap(base.refs()); + } + + @Override + public UpdateSnapshotRefs tag(String name, long snapshotId) { + Preconditions.checkArgument(name != null, "Tag name must not be null"); + Preconditions.checkArgument(base.snapshot(snapshotId) != null, "Cannot find snapshot with ID: %s", snapshotId); + Preconditions.checkArgument(!refs.containsKey(name), "Cannot tag snapshot, ref already exists: %s", name); + + refs.put(name, SnapshotRef.tagBuilder(snapshotId).build()); + return this; + } + + @Override + public UpdateSnapshotRefs branch(String name, long snapshotId) { + Preconditions.checkArgument(name != null, "Branch name must not be null"); + Preconditions.checkArgument(base.snapshot(snapshotId) != null, "Cannot find snapshot with ID: %s", snapshotId); + Preconditions.checkArgument(!refs.containsKey(name), "Cannot create branch, ref already exists: %s", name); Review comment: From the snapshot producer perspective, I was thinking 2 cases: 1) Append to branch: ``` table.newRewrite() .rewriteFiles(Sets.newSet(FILE_A), Sets.newSet(FILE_B)) .atBranch("beta") ``` In this case, I would say both use cases make sense: 1. user might want to fail when committing to a non-existing branch, because that branch was deleted by someone else. This could be satisfied by the current API by removing old branch and update it with the same info, a bit cumbersome but still works. 2. user might want to create the branch with the new snapshot ID, this could also be done by the current API by first checking branch existence and then do a create after snapshot ID is committed. 2) Add tag: ``` table.newRewrite() .rewriteFiles(Sets.newSet(FILE_A), Sets.newSet(FILE_B)) .tag("2022-01-01-compacted"） ``` In this case, tagging with an existing name is likely a system issue as there should only be a single process doing the commit and tag it. This could be identified by the current API. So from my perspective the current API is enough, although the branch interface is not straight forward for people to just update the branch head. I would imagine it to be a frequent operation, so might worth adding that. Regarding the builder API, currently it's following the pattern of property update. I think `set` and `remove` would be beneficial for performing conditional update for specific refs and avoid full metadata check as we develop the rest catalog, do you think there is any other APIs for the builder that would be more suitable? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
