Catalog: return Iceberg snapshot log based on Nessie commit history #10033

snazy · 2024-12-03T13:06:09Z

The current behavior of Nessie's Iceberg REST is to return only the most recent Iceberg snapshot. However, this seems to conflict with some Iceberg operations, which are not only maintenance operations, but rather related to "merge on read" / (equality) deletes.

This change changes Nessie's behavior by returning older snapshots from a load-table and update-table operations.

Register-table operation however do not change, because only the latest snapshot is actually imported. The behavior does change by returning an error if the table to be registered has more than 1 snapshots.

Fixes #10013
Fixes #9969

dimas-b

LGTM 👍 A couple of minor comments below

dimas-b · 2024-12-03T21:57:41Z

...og/service/impl/src/main/java/org/projectnessie/catalog/service/impl/CatalogServiceImpl.java

+    var contentHistory =
+        treeService(apiContext)
+            .getContentHistory(
+                key, reference.name(), reference.hashWithRelativeSpec(), requestMeta);


I wonder if it makes sense to limit the history to 2 by default? I suppose most client-side Iceberg operations need at most one parent snapshot.

Sadly, we have to return all the previous snapshots, because we cannot know which ones are needed.

How will it work with Nessie GC? We never remove snapshots from iceberg metadata, IIRC 🤔

...og/service/impl/src/main/java/org/projectnessie/catalog/service/impl/CatalogServiceImpl.java

The current behavior of Nessie's Iceberg REST is to return only the most recent Iceberg snapshot. However, this seems to conflict with some Iceberg operations, which are not only maintenance operations, but rather related to "merge on read" / (equality) deletes. This change changes Nessie's behavior by returning older snapshots from a load-table and update-table operations. Register-table operation however do not change, because only the latest snapshot is actually imported. The behavior does change by returning an error if the table to be registered has more than 1 snapshots. Fixes projectnessie#10013 Fixes projectnessie#9969

snazy · 2024-12-04T10:54:00Z

Ouch. So, a single Iceberg transaction can actually add multiple snapshots - but we only have one commit in Nessie - but need to be able to access both snapshots...
Need to think more about this, setting this change back to draft state.

snazy · 2024-12-04T15:06:08Z

So, I hope that all tests are happy again.

The changes became more intrusive, but it should all work now.

Had to let the register-table endpoint not accept Nessie catalog URIs anymore, because that would lead to two Nessie tables pointing to the same snapshot (NessieTableSnapshot) - and it's actually not good in general to have different tables work in the same (storage) location. OTOH externally registered tables now get all snapshots imported via register-table. The check is a bit racy, but I suspect, it's okay-ish / better-than-nothing ™️ .

dimas-b · 2024-12-04T15:42:31Z

...og/service/impl/src/main/java/org/projectnessie/catalog/service/impl/CatalogServiceImpl.java

+              var implicitIcebergSnapshot =
+                  snapFuture != null
+                      ? implicitIcebergSnapshot(
+                          (NessieTableSnapshot) snapFuture.get(0, TimeUnit.SECONDS))


snapFuture.join()?

Why? It's already finished

Exactly. It's just a nicer method for this case, does not have any timeout parameters and does not have checked exceptions.

dimas-b · 2024-12-04T15:49:03Z

catalog/service/common/src/main/java/org/projectnessie/catalog/service/api/CatalogService.java

-    URI icebergSnapshot(
-        Reference effectiveReference, ContentKey key, NessieEntitySnapshot<?> snapshot);
-  }
+  boolean checkIcebergSnapshotPresent(String metadataLocation, long versionId);


versionId -> snapshotId (for clarity)?

The property in IcebergContent is versionId

But the method checks for snapshots and it's not apparent that the versionId parameter here should be the snapshot ID.

dimas-b · 2024-12-04T16:10:46Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

+      if (snapId == iceberg.currentSnapshotId()) {
+        continue;
+      }
+      if (previous == null) {


Is previous ever non-null? IJ shows all non-test call paths leading to null 🤔

Those calls come from importIcebergTable... So, essentially if someone commits via Nessie API, each NessieTableSnapshot (after importing) will have duplicate "implicit" snapshot lists. WDYT?

Yes - unfortunate.

why keep previous if it is always null in production call paths?

dimas-b · 2024-12-04T16:16:59Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

@@ -1599,6 +1626,23 @@ public static void addSnapshot(AddSnapshot u, IcebergTableMetadataUpdateState st
        .icebergManifestListLocation(icebergSnapshot.manifestList())
        .icebergManifestFileLocations(icebergSnapshot.manifests());

+    // If another Iceberg snapshot was add in this same update-transaction, memoize the previous


nit: add -> added

adutra · 2024-12-09T09:25:35Z

CHANGELOG.md

+- Nessie now returns the snapshot history in the `snapshots` and `snapshot-log` attributes of an Iceberg
+  table-metadata retrieved via Iceberg REST for table changes that have been committed via Iceberg REST
+  to Nessie 0.101.0 or newer. Commits made using older Nessie versions will not return older snapshots.
+- Generally, not only for Nessie, It is recommended to keep the number of snapshots maintained in an


Suggested change

- Generally, not only for Nessie, It is recommended to keep the number of snapshots maintained in an

- Generally, not only for Nessie, it is recommended to keep the number of snapshots maintained in an

adutra · 2024-12-09T09:27:27Z

catalog/model/src/main/java/org/projectnessie/catalog/model/snapshot/NessieTableSnapshot.java

+
+  /**
+   * Contains Iceberg snapshots for which no explicit Nessie commit exists. We need to memoize those
+   * snapshots here in case a single Iceberg transaction add multiple snapshots.


Suggested change

* snapshots here in case a single Iceberg transaction add multiple snapshots.

* snapshots here in case a single Iceberg transaction adds multiple snapshots.

adutra · 2024-12-09T09:50:17Z

...ore/src/test/java/org/projectnessie/versioned/storage/versionstore/TestVersionStoreImpl.java

+            // iterator
+            tuple(keyRenamed, "rename 1", hashRename1.getCommitHash(), contentRename1),
+            tuple(key, "update 2", hashUpdate2.getCommitHash(), contentUpdate2),
+            tuple(key, "create-other", hashCreateOther.getCommitHash(), contentUpdate1),


Why is this commit returned, since it does not change key? Imho it should return the "update 1" commit message here since that is the commit that introduced contentUpdate1.

snazy force-pushed the iceberg-include-previous-snapshots branch 5 times, most recently from e0cac8c to 614f88c Compare December 3, 2024 15:56

snazy marked this pull request as ready for review December 3, 2024 16:14

dimas-b reviewed Dec 3, 2024

View reviewed changes

snazy added this to the 0.101.0 milestone Dec 4, 2024

snazy marked this pull request as draft December 4, 2024 10:54

snazy added 2 commits December 4, 2024 12:30

collector

c3d26a1

memoize "implicit" snapshots

134a13c

snazy force-pushed the iceberg-include-previous-snapshots branch from 614f88c to 134a13c Compare December 4, 2024 14:30

snazy added 2 commits December 4, 2024 16:00

Fix tests

27af5d1

add some snapshot-id validation

e9d0abd

snazy force-pushed the iceberg-include-previous-snapshots branch from 8673633 to e9d0abd Compare December 4, 2024 15:02

snazy marked this pull request as ready for review December 4, 2024 15:02

dimas-b reviewed Dec 4, 2024

View reviewed changes

snazy modified the milestones: 0.101.0, 0.102.0 Dec 5, 2024

adutra reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catalog: return Iceberg snapshot log based on Nessie commit history #10033

Catalog: return Iceberg snapshot log based on Nessie commit history #10033

snazy commented Dec 3, 2024

dimas-b left a comment

dimas-b Dec 3, 2024

snazy Dec 4, 2024

dimas-b Dec 4, 2024

snazy commented Dec 4, 2024

snazy commented Dec 4, 2024

dimas-b Dec 4, 2024

snazy Dec 4, 2024

dimas-b Dec 4, 2024 •

edited

Loading

dimas-b Dec 4, 2024

snazy Dec 4, 2024

dimas-b Dec 4, 2024

dimas-b Dec 4, 2024

dimas-b Dec 4, 2024

snazy Dec 4, 2024

dimas-b Dec 4, 2024

dimas-b Dec 4, 2024

adutra Dec 9, 2024

adutra Dec 9, 2024

adutra Dec 9, 2024

	- Generally, not only for Nessie, It is recommended to keep the number of snapshots maintained in an
	- Generally, not only for Nessie, it is recommended to keep the number of snapshots maintained in an

	* snapshots here in case a single Iceberg transaction add multiple snapshots.
	* snapshots here in case a single Iceberg transaction adds multiple snapshots.

Catalog: return Iceberg snapshot log based on Nessie commit history #10033

Are you sure you want to change the base?

Catalog: return Iceberg snapshot log based on Nessie commit history #10033

Conversation

snazy commented Dec 3, 2024

dimas-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snazy commented Dec 4, 2024

snazy commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Dec 4, 2024 •

edited

Loading