Core: add snapshot reference to table metadata #3104

jackye1995 · 2021-09-13T03:05:51Z

Co-authored-by: @hameizi 1249369293@qq.com

continuation of #2961 after some design discussion, still WIP but I'd like to first publish it to receive some feedback and continue with the effort.

https://docs.google.com/document/d/1PvxK_0ebEoX3s7nS6-LOJJZdBYr_olTWH9oepNUfJ-A/edit#

@rdblue @RussellSpitzer @szehon-ho @stevenzwu

szehon-ho

Thanks for continuing the effort, and thanks for renaming the snapshot-annotation values to my suggestion on the doc, I think it sounds more reasonable. I am still not 100% sure it is the way to go (adding this extra state, vs exposing configs/lambda function on expireSnapshot method itself), but I'm ok if everyone agrees. Left some comments in a first pass

szehon-ho · 2021-09-17T04:59:32Z

api/src/main/java/org/apache/iceberg/SnapshotAnnotation.java

+  }
+
+  @Override
+  public boolean equals(Object o) {


Maybe I missed something, but what is different than default equals/hashcode?

szehon-ho · 2021-09-17T05:04:16Z

core/src/main/java/org/apache/iceberg/SnapshotAnnotationParser.java

+    }
+  }
+
+  public static void toJson(SnapshotAnnotation annotation, JsonGenerator generator) throws IOException {


Can we have a better name/signature for this, toJson infers it returns a string (was confused when reading the caller code). Maybe writeSnapshotAnnotation(JsonGenerator generator, SnapshotAnnotation annotation) if not too redundant

And also do we need make it public?

szehon-ho · 2021-09-17T05:07:46Z

core/src/main/java/org/apache/iceberg/util/JsonUtil.java

+      return null;
+    }
+
+    Preconditions.checkArgument(node.has(property), "Cannot parse missing long %s", property);


No point anymore to this check anymore , right ?

correct, updated

rdblue · 2021-09-17T19:06:18Z

I think that a feature like this should be more understandable and straightforward. If our overall goal is to enable branching and tagging, then I think it makes the most sense to do that directly rather than introducing a similar concept like annotations.

jackye1995 · 2021-10-27T04:32:07Z

@rdblue @szehon-ho @nastra updated based on the last discussion, could you take a look?

jackye1995 · 2021-10-27T06:10:56Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-    // update the snapshot log
+    // update the snapshot log and refs
    Set<Long> validIds = Sets.newHashSet(Iterables.transform(filtered, Snapshot::snapshotId));
+    List<SnapshotReference> newRefs = refs.stream()


my thought is that remove snapshot should work as usual without failing for snapshot with ref, and predicate should be formulated based on ref information.

nastra · 2021-11-04T06:20:14Z

api/src/main/java/org/apache/iceberg/SnapshotReference.java

+  }
+
+  /**
+   * Returns the minimum number of snapshots to keep for a BRANCH, or null for a TAG


maybe minSnapshotsToKeep can be 0 instead of null for a TAG? Then you could use the primitive int type rather than Integer

RussellSpitzer · 2021-11-04T14:59:41Z

api/src/main/java/org/apache/iceberg/SnapshotReference.java

+      this.type = type;
+    }
+
+    public Builder withMinSnapshotsToKeep(Integer value) {


Are these two properties for use in "expire snapshots?

will be used once we added it to spec

RussellSpitzer · 2021-11-05T15:30:39Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

  private final Map<Integer, SortOrder> sortOrdersById;
  private final List<HistoryEntry> snapshotLog;
  private final List<MetadataLogEntry> previousFiles;
+  private final List<SnapshotReference> refs;


This is where I am a little confused. Does every Metadata.json contain the full branch and snapshot references for all other branches and snapshots?

Yes that's the current plan, the metadata.json always contains all the snapshots and all references.

If we do so, how are we gonna support snapshot history ? metadata tables per reference should show and use its own snapshots right ?

oh, I think we should discuss this in Spec PR, (#3425)

After thinking more on this, I think current snapshot will have parent id. so we can still form a history snapshots per branch just by keeping the current snapshot id. So, no need spec change.

But we have to change code for handling metadata tables per reference using above information.

rymurr · 2021-11-17T12:44:04Z

Hey @jackye1995 I added a bunch of comments to the design doc. Do you consider this more up to date now? Should I commnet/review here instead?

jackye1995 · 2021-12-08T23:23:16Z

I have updated this PR based on the TableMetadata builder refactoring. it is quite hard to separate the update logic out of this, so I just included the changes in #3612 and marked @hameizi as coauthor.

jackye1995 · 2021-12-08T23:29:46Z

core/src/main/java/org/apache/iceberg/BaseUpdateSnapshotRefs.java

+
+  private final TableOperations ops;
+  private final TableMetadata base;
+  private final Map<String, SnapshotRef> refs;


Unlike properties update, I just track the final result of refs map here, and we will not retry during commit phase to avoid concurrent updates of refs.

jackye1995 · 2021-12-08T23:35:18Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+   * @param currentRefs current table refs list
+   * @return updated refs list
+   */
+  private static Map<String, SnapshotRef> refsWithMainBranch(long snapshotId, Map<String, SnapshotRef> currentRefs) {


The snapshot ID here can be -1. I have been thinking if we should keep the main branch ref in case there is no main branch head after a replace operation. But there might be retention policy set at the main branch, and removing the main branch will result in deleting that information.

I've been thinking about how awkward this is for a while. We may want to just create an empty snapshot when creating a table. That would clean up using -1.

Yes +1 for creating an empty snapshot when creating a table

hameizi · 2021-12-21T04:27:29Z

@rdblue Is there any more concern about this PR?

rdblue · 2022-01-09T23:10:38Z

api/src/main/java/org/apache/iceberg/SnapshotRef.java

+    private Long maxRefAgeMs;
+
+    Builder(SnapshotRefType type) {
+      ValidationException.check(type != null, "Snapshot reference type must not be null");


I think most builders use IllegalArgumentException to reject arguments that aren't allowed and ValidationException to catch inconsistencies. The ValidationException cases that are thrown in build are good examples of when a ValidationException is a good idea because, for example, minSnapshotsToKeep doesn't make sense with a tag.

rdblue · 2022-01-09T23:12:23Z

api/src/main/java/org/apache/iceberg/SnapshotRef.java

+    public SnapshotRef build() {
+      if (type.equals(SnapshotRefType.TAG)) {
+        ValidationException.check(minSnapshotsToKeep == null,
+            "TAG type snapshot reference does not support setting minSnapshotsToKeep");


Can we use "Tag" instead of "TAG type snapshot reference"? I think people are going to be more familiar with the friendly names, "tag" and "branch", so we should use those rather than focusing on the generalized "reference".

rdblue · 2022-01-09T23:13:16Z

api/src/main/java/org/apache/iceberg/SnapshotRef.java

+      } else {
+        if (minSnapshotsToKeep != null) {
+          ValidationException.check(minSnapshotsToKeep > 0,
+              "Min snapshots to keep must be greater than 0");


This could be ValidationException.check(minSnapshotsToKeep == null || minSnapshotsToKeep > 0, ...) rather than using an if block.

rdblue · 2022-01-09T23:15:27Z

api/src/main/java/org/apache/iceberg/Table.java

+   * @return a set of {@link SnapshotRef snapshot references} of a snapshot.
+   *         Note that when there is no ref, it returns an empty set but not null.
+   */
+  Set<SnapshotRef> refs(long snapshotId);


Why have a method for returning refs by snapshot but not a ref by name?

I was thinking it's useful in subsequent PRs for updating the snapshot expiration logic, but we can remove this for now and just have a method for querying the snapshot of a specific ref name.

rdblue · 2022-01-09T23:19:48Z

api/src/main/java/org/apache/iceberg/Table.java

+   *
+   * @return a new {@link UpdateSnapshotRefs}
+   */
+  UpdateSnapshotRefs updateRefs();


How do you think the other table mutations should handle refs? For example, an append might look like this:

table.newAppend() .appendFile(someFile) .commit();

Should we add an optional branch to all of the SnapshotProducer operations?

table.newAppend() .appendFile(someFile) .branch("some_branch") .commit();

Curious what you've been thinking.

Yes I do have exactly the same thought, where append will go to the main branch by default and an override is allowed. I think a follow-up PR is needed for this feature.

rdblue · 2022-01-09T23:23:50Z

api/src/main/java/org/apache/iceberg/SnapshotRef.java

+    return maxRefAgeMs;
+  }
+
+  public static Builder builderForTag(long snapshotId) {


What about a slightly more direct name scheme, tagBuilder and branchBuilder?

rdblue · 2022-01-09T23:24:47Z

api/src/main/java/org/apache/iceberg/SnapshotRef.java

+
+  public static Builder builderFrom(SnapshotRef ref) {
+    return new Builder(ref.type())
+        .snapshotId(ref.snapshotId())


Why use a builder method for snapshot ID when it is passed in through the factory method? What happens when you try to change the snapshot ID or set it to null? I think it may be simpler just to make this a private final field and remove the builder method for it.

rdblue · 2022-01-09T23:27:42Z

core/src/main/java/org/apache/iceberg/SerializableTable.java


+  @Override
+  public Map<String, SnapshotRef> refs() {
+    return lazyTable().refs();


Seems reasonable to read the table metadata for this. I doubt serialized tables will be accessing refs.

rdblue · 2022-01-09T23:29:08Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

        currentSnapshotId < 0 || snapshotsById.containsKey(currentSnapshotId),
        "Invalid table metadata: Cannot find current version");
+
+


Nit: unnecessary whitespace change.

rdblue · 2022-01-09T23:29:39Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

  private final List<HistoryEntry> snapshotLog;
  private final List<MetadataLogEntry> previousFiles;
+  private final Map<String, SnapshotRef> refs;
+  private final SetMultimap<Long, SnapshotRef> refsById;


I don't understand why these are indexed by snapshot rather than name. Is that a common pattern?

rdblue · 2022-01-09T23:31:37Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+    List<Snapshot> toRemove = snapshots.stream()
+        .filter(removeIf)
+        .filter(snapshot -> !refsById.containsKey(snapshot.snapshotId()))
+        .collect(Collectors.toList());


I think it is probably time to remove this method, or at least to stop using it. Instead, I'd probably just produce a list of snapshot IDs to remove. That fits better with the new builder style.

added #3900 to track this

rdblue · 2022-01-09T23:32:28Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

  }

+  public TableMetadata replaceRefs(Map<String, SnapshotRef> newRefs) {
+


Nit: unnecessary whitespace starting the function.

rdblue · 2022-01-10T18:47:33Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+  private Map<String, SnapshotRef> validateAndCompleteRefs(Map<String, SnapshotRef> inputRefs) {
+    for (SnapshotRef ref : inputRefs.values()) {
+      Preconditions.checkArgument(snapshotsById.containsKey(ref.snapshotId()) || ref.snapshotId() == -1,
+          "Snapshot reference %s does not exist in the existing snapshots list", ref);


Do we also want to validate that only main can be -1?

amogh-jahagirdar · 2022-01-11T00:29:34Z

Hi all, I have been discussing snapshot lifecycle management with @jackye1995 and if possible would like to take over addressing any remaining comments for these changes as well as any other potential, relevant implementation improvements. @jackye1995 would this be possible? Would raise another PR and mark @jackye1995 and @hameizi 1249369293@qq.com as co-authors.

jackye1995 · 2022-01-11T01:00:01Z

Sure, I am a bit low in capacity for implementation recently, please feel free to open a new PR to push the progress of this PR forward and I will then close this one review it, thank you very much @amogh-jahagirdar !

…e metadata in apache#3104

…e metadata in apache#3104 Remove unused interface method for getting all snapshot refs for a given snapshot id.

Followed up on comments on apache#3104 Remove unused interface method for getting all snapshot refs for a given snapshot id. TableMetadata removeSnapshots accepts a list of snapshot IDs. It's up to callers to build a list of the expected snapshot IDs to remove. Added validation that a -1 snapshot id can only happen for the main branch reference. Co-authored-by: wangzeyu <1249369293@qq.com> Co-authored-by: Jack Ye <yzhaoqin@amazon.com>

RussellSpitzer · 2022-01-13T16:20:58Z

@jackye1995 So we should focus on @amogh-jahagirdar s PR now? Just want to close out this one if that's the case.

jackye1995 · 2022-01-13T18:36:33Z

@RussellSpitzer yes, let me close this one in favor of #3883

Followed up on comments on apache#3104 Remove unused interface method for getting all snapshot refs for a given snapshot id. TableMetadata removeSnapshots accepts a list of snapshot IDs. It's up to callers to build a list of the expected snapshot IDs to remove. Added validation that a -1 snapshot id can only happen for the main branch reference. Co-authored-by: wangzeyu <1249369293@qq.com> Co-authored-by: Jack Ye <yzhaoqin@amazon.com> Cleanup

Followed up on comments on apache#3104 Remove unused interface method for getting all snapshot refs for a given snapshot id. TableMetadata removeSnapshots accepts a list of snapshot IDs. It's up to callers to build a list of the expected snapshot IDs to remove. Added validation that a -1 snapshot id can only happen for the main branch reference. Co-authored-by: wangzeyu <1249369293@qq.com> Co-authored-by: Jack Ye <yzhaoqin@amazon.com>

Followed up on comments on apache#3104 Remove unused interface method for getting at all snapshot refs for a given snapshot id. TableMetadata removeSnapshots accepts a list of snapshot IDs. It's up to callers to build a list of the expected snapshot IDs to remove. Added validation that a -1 snapshot id can only happen for the main branch reference. Co-authored-by: wangzeyu <1249369293@qq.com> Co-authored-by: Jack Ye <yzhaoqin@amazon.com>

github-actions bot added API core labels Sep 13, 2021

szehon-ho reviewed Sep 17, 2021

View reviewed changes

jackye1995 changed the title ~~Core: add snapshot annotation to table metadata (WIP)~~ Core: add snapshot reference to table metadata Oct 27, 2021

jackye1995 commented Oct 27, 2021

View reviewed changes

nastra reviewed Nov 4, 2021

View reviewed changes

RussellSpitzer reviewed Nov 4, 2021

View reviewed changes

RussellSpitzer reviewed Nov 5, 2021

View reviewed changes

ajantha-bhat mentioned this pull request Nov 17, 2021

New global state is breaking the Alter table scenarios projectnessie/nessie#2670

Closed

hameizi mentioned this pull request Nov 26, 2021

Add UpdateSnapshotReference #3612

Closed

jackye1995 commented Dec 8, 2021

View reviewed changes

Core: add snapshot reference to table metadata

7fe760e

jackye1995 requested a review from rdblue December 13, 2021 19:50

rdblue reviewed Jan 9, 2022

View reviewed changes

rdblue reviewed Jan 10, 2022

View reviewed changes

amogh-jahagirdar added a commit to amogh-jahagirdar/iceberg that referenced this pull request Jan 12, 2022

Core: Addressing comments for snapshot references being added to tabl…

a3d309c

…e metadata in apache#3104

amogh-jahagirdar mentioned this pull request Jan 12, 2022

Core: add snapshot reference to table metadata #3883

Closed

jackye1995 closed this Jan 13, 2022

jackye1995 mentioned this pull request Jan 13, 2022

Clarify usage of ValidationException vs IllegalArgumentException #3895

Closed

		currentSnapshotId < 0 \|\| snapshotsById.containsKey(currentSnapshotId),
		"Invalid table metadata: Cannot find current version");

		}

		public TableMetadata replaceRefs(Map<String, SnapshotRef> newRefs) {

Core: add snapshot reference to table metadata #3104

Core: add snapshot reference to table metadata #3104

Uh oh!

Conversation

jackye1995 commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szehon-ho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Sep 17, 2021

Uh oh!

jackye1995 commented Oct 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rymurr commented Nov 17, 2021

Uh oh!

jackye1995 commented Dec 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hameizi commented Dec 21, 2021

Uh oh!

rdblue Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jackye1995 commented Sep 13, 2021 •

edited

Loading

szehon-ho left a comment •

edited

Loading

ajantha-bhat Nov 17, 2021 •

edited

Loading

rdblue Jan 9, 2022 •

edited

Loading