Bump Nessie to 0.15.1 + related changes #3257

snazy · 2021-10-08T18:33:35Z

Use new NessieApiV1
Update reference syntax to table-identifier ( '@' reference-name )? ( '#' pointer )? (for Nessie-Iceberg-GC)
Slightly nicer commit message from NessieTableOperations

Contributes to #3243

/cc @nastra

snazy · 2021-10-10T10:10:39Z

Moved to draft status due to projectnessie/nessie#2312

rdblue · 2021-10-10T17:54:09Z

@nastra, can you take a look?

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

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

rdblue · 2021-10-10T17:59:52Z

This appears to change the relationship between Nessie and Iceberg metadata. Can you please explain what you're doing here and why it is needed? What is the model for Nessie and Iceberg tracking now? I thought it was that Nessie tracked different Iceberg metadata files. Is that no longer the case and Nessie is tracking structures within Iceberg?

snazy · 2021-10-14T09:22:44Z

@rdblue Sorry for the late reply. Yes, this one changes the relationship between Nessie and Iceberg metadata.

TL;DR the changes shall ensure that changes against the same table on different branches can later be merged together without having duplicate column-IDs or partition-IDs or the like.

You're right, initially (in the "early Nessie days"), every Nessie commit held a pointer to the table-metadata. This works fine until you reference the same table on different branches and perform e.g. schema changes (think: ALTER TABLE ADD COLUMN) on both branches, which leads to duplicate column-ids (in other words: same column id used for different columns), which then can lead to data corruption when branches get merged.

So the initial approach in this PR was to maintain table-metadata across all branches and "just" reference the snapshot-ID from in Nessie commits, which led to other issues (not explaining it here further, but schema changes became an issue again).

The current approach is more like the initial approach: have the pointer to table-metadata in Nessie commits but track state that's important across all Nessie branches (e.g. last-column-ID) globally. It's currently implemented via additional functionality in TableMetadata to retrieve the "global state" (last-column-ID, last-used-partition-ID, last-assigned-sequence-ID) as an object that's opaque to Nessie plus functionality to update a TableMetadata using that "global state".

jackye1995 · 2021-10-23T00:31:36Z

It's still unclear to me how this would solve the merge conflict if you have 2 columns updated in different branches. For example, if I have branch A that added column c1 as an int column, branch B also added column c1 as a string column, even if they have different ID, it is unclear if the user's intention is to keep one c1 (and we don't know which one to keep), or keep both (which is not possible due to duplicated column name).

snazy · 2021-10-23T10:12:08Z

It's still unclear to me how this would solve the merge conflict if you have 2 columns updated in different branches. For example, if I have branch A that added column c1 as an int column, branch B also added column c1 as a string column, even if they have different ID, it is unclear if the user's intention is to keep one c1 (and we don't know which one to keep), or keep both (which is not possible due to duplicated column name).

Yes, that’s a known concern. It would have to lead in an merge conflict error. However, it could be resolved by the user by renaming one of those.

harshm-dev

Thanks for posting this change.

harshm-dev · 2021-10-27T06:16:38Z

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

+ * of the relevant ID generators across all references. This allows cherry-picking and merging
+ * specific changes onto different branches.
+ */
+public final class TableIdGenerators {


nit: The class isn't really generating any IDs, just holding the state instead. Using the suffix "IdGenerators" is confusing.

harshm-dev · 2021-10-27T06:22:24Z

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

+  public static String toJson(TableIdGenerators idGenerators) {
+    try (StringWriter writer = new StringWriter()) {
+      JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
+      idGeneratorsJson(idGenerators, generator);


Why not just directly json serialise/de-serialise the object?
PropertyNames/validations can be achieved via appropriate annotations.

harshm-dev · 2021-10-27T06:26:51Z

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

+  }
+
+  // intentionally package-private - use TableMetadata.getIdGenerators() or parse via TableIdGeneratorsParser
+  static TableIdGenerators of(int formatVersion, int lastColumnId, int lastAssignedPartitionId,


Just wondering why constructor cannot be invoked directly.
What's the intention behind using a factory method?

harshm-dev · 2021-10-27T07:06:40Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

-      metadataLocation = table.getMetadataLocation();
+      Contents contents = api.getContents().key(key).reference(reference.getReference()).get()
+          .get(key);
+      LOG.info("Contents '{}' at '{}': {}", key, reference.getReference(), contents);


This should be a debug level log.

harshm-dev · 2021-10-27T08:34:43Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

  private static final Logger logger = LoggerFactory.getLogger(NessieCatalog.class);
  private static final Joiner SLASH = Joiner.on("/");
-  private NessieClient client;
+  private NessieApiV1 api;


client is relatively a better variable name at the consumption. clientApi could be an option since the class name is also changing.

harshm-dev · 2021-10-27T08:40:10Z

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

    this.shouldRefresh = false;
  }

+  protected TableMetadata validateRefreshedMetadata(TableMetadata metadata) {


From the code below, the method seems to be supplementing metadata with global state IDs and returning a mutated object.
Naming this method as validateXX doesn't feel correct.

agreed. I would go further and say this additional protected method feels fragile and not a great use of class hierarchies. I would prefer NessieTableOperations just reimplements the above refreshFromMetadataLocation completely rather than adding this shim

that would require making a bunch of fields protected in the base class. I renamed the method to maybeValidateAndUpdateMetadata. Hopefully that makes its purpose clearer

harshm-dev · 2021-10-27T08:44:57Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

  protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
-    TableReference pti = TableReference.parse(tableIdentifier);
+    TableReference tr = TableReference.parse(tableIdentifier.name());
+    if (tr.hasTimestamp()) {


Better to use Precoditions.checkArgument(..)

harshm-dev · 2021-10-27T09:05:50Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

-          .commitMeta(NessieUtil.buildCommitMetadata("iceberg commit", catalogOptions)).build();
-      Branch branch = client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),
-          reference.getHash(), op);
+      LOG.info("Committing '{}' against '{}': {}", key, reference.getReference(), newTable);


debug level please

harshm-dev · 2021-10-27T09:07:46Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

+      commitMessage = String.format("Iceberg commit against %s", tableName());
+    }
+
+    return commitMeta.message(commitMessage);


This is useful, thx!

However, the function is taking multiple responsibilities - initialising a builder, setting properties apart from identifying commit message.
I think we should separate them out, an example version of the consuming side of the code -

ImmutableCommitMeta.Builder commitMetaBuilder = ImmutableCommitMeta.builder(); addOperationDetails(commitMetaBuilder, base, metadata); // this method NessieUtils.addCatalogOptions(commitMetaBuilder, catalogOptions);

I also find this rather confusing to read, some rather complicated booleans floating around. This seems like complexity in the name of removing code duplication

omarsmak · 2021-10-27T10:09:02Z

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

  }

  public void updateReference(Reference ref) {
+    if (!mutable) {


I think Preconditions.checkState(mutable, "Hash references cannot be updated.") would be nicer.

omarsmak · 2021-10-27T10:09:59Z

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

  public void checkMutable() {
-    if (!isBranch()) {
-      throw new IllegalArgumentException("You can only mutate tables when using a branch.");
+    if (!mutable) {


Here is as well, perhaps Preconditions.checkArgument would be nicer

omarsmak · 2021-10-27T10:13:10Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

-        .addOperations(
-            ImmutablePut.builder().key(NessieUtil.toKey(to)).contents(existingFromTable).build(),
-            ImmutableDelete.builder().key(NessieUtil.toKey(from)).build())
+    CommitMultipleOperationsBuilder op = api.commitMultipleOperations()


nit: Can we change op to something plural like operations to have more readable.

omarsmak · 2021-10-27T10:14:45Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

-            Branch branch = client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),
-                reference.getHash(), c);
+          .onFailure((o, exception) -> refresh())
+          .run(o -> {


nit: perhaps singular operation would make sense instead of o here?

nastra

since Robert is out this week, I made some changes according to the review feedback

nastra · 2021-11-17T08:17:03Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

  protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
-    TableReference pti = TableReference.parse(tableIdentifier);
+    TableReference tr = TableReference.parse(tableIdentifier.name());
+    if (tr.hasTimestamp()) {


nastra · 2021-11-17T08:18:09Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

-        .addOperations(
-            ImmutablePut.builder().key(NessieUtil.toKey(to)).contents(existingFromTable).build(),
-            ImmutableDelete.builder().key(NessieUtil.toKey(from)).build())
+    CommitMultipleOperationsBuilder op = api.commitMultipleOperations()


nastra · 2021-11-17T08:18:17Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

-            Branch branch = client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),
-                reference.getHash(), c);
+          .onFailure((o, exception) -> refresh())
+          .run(o -> {


nastra · 2021-11-17T08:19:57Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

-          .commitMeta(NessieUtil.buildCommitMetadata("iceberg commit", catalogOptions)).build();
-      Branch branch = client.getTreeApi().commitMultipleOperations(reference.getAsBranch().getName(),
-          reference.getHash(), op);
+      LOG.info("Committing '{}' against '{}': {}", key, reference.getReference(), newTable);


nastra · 2021-11-17T08:23:57Z

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

  }

  public void updateReference(Reference ref) {
+    if (!mutable) {


nastra · 2021-11-17T08:24:03Z

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

  public void checkMutable() {
-    if (!isBranch()) {
-      throw new IllegalArgumentException("You can only mutate tables when using a branch.");
+    if (!mutable) {


rymurr

Added some comments, I think these changes could be architected better.

Also: this is a fairly fundamental change to how nessie and iceberg interact. I would expect more explanation of the change and its motivation and probably some docs changes (or links to nessie docs explaining iceberg/nessie interaction)

rymurr · 2021-11-17T09:44:38Z

build.gradle

    implementation project(':iceberg-core')
    implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
    implementation "org.projectnessie:nessie-client"
+    implementation "com.fasterxml.jackson.core:jackson-databind"


is this a new dependency or just being explicit about an existing one? If new does this change the size of the runtime jars?

rymurr · 2021-11-17T09:47:51Z

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

    this.shouldRefresh = false;
  }

+  protected TableMetadata validateRefreshedMetadata(TableMetadata metadata) {


agreed. I would go further and say this additional protected method feels fragile and not a great use of class hierarchies. I would prefer NessieTableOperations just reimplements the above refreshFromMetadataLocation completely rather than adding this shim

rymurr · 2021-11-17T09:48:56Z

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

        snapshots, newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
  }

+  public TableMetadata withUpdatedSnapshotAndSchema(long snapshotId, int schemaId, int sortOrderId, int specId) {


I think these would be more useful as separate methods. There is no way to only update snapshotId for example and all 4 args have to be given valid values or it throws. Seems overly restrictive and not very composable.

I think that things would be easier with the changes from #2957, where we could just call the builder and only update the actual fields that we need. Then we wouldn't need to have the withUpdatedSnapshotAndSchema method in TableMetadata and could do everything in the NessieCatalog where we need it. Maybe we should go forward with this as is right now and then refactor/improve this once #2957 is merged?

For #2957 I was waiting for more clarity around the REST catalog to design the table metadata update builder semantics, so it probably won't be merged very soon. But I agree with Ryan that this seems like a very restrictive method, and the intention of the operation is unclear comparing to the other update methods (although I know how it fits in the Nessie context).

To update the schemaId, sortOrderId, specId to a specific version, I think it makes more sense to expose specific APIs in related update classes instead of having everything done through a separated method. What do you think?

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

rymurr · 2021-11-17T09:59:01Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java


-  TreeApi getTreeApi() {
-    return client.getTreeApi();
+  public NessieApiV1 getApi() {


why is this public? I don't think we should be exposing this

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

rymurr · 2021-11-17T10:02:20Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

+      commitMessage = String.format("Iceberg commit against %s", tableName());
+    }
+
+    return commitMeta.message(commitMessage);


I also find this rather confusing to read, some rather complicated booleans floating around. This seems like complexity in the name of removing code duplication

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

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

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

rymurr

LGTM thanks @snazy and @nastra

* Use new NessieApiV1 * Update reference syntax to `table-identifier ( '@' reference-name )? ( '#' pointer )?` (for Nessie-Iceberg-GC) * Slightly nicer commit message from `NessieTableOperations` * Use new `TableIdGenerators` as the "global state" tracked in Nessie

site/mkdocs.yml

Co-authored-by: Ajantha Bhat <ajanthabhat@gmail.com>

rdblue · 2021-12-04T00:16:16Z

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

          .shouldRetryTest(shouldRetry)
-          .run(metadataLocation -> newMetadata.set(
-              TableMetadataParser.read(io(), metadataLocation)));
+          .run(metadataLocation -> newMetadata.set(metadataLoader.apply(metadataLocation)));


@rymurr, I'm just catching up on this PR, sorry to be late.

I'm curious why this uses a Function passed into refresh rather than defining a load metadata method. The function passed in from NessieTableOperations is actually a method reference, so couldn't this just be a call to that method, loadTableMetadata?

.run(metadataLocation -> loadTableMetadata(metadataLocation)); ... public TableMetadata loadTableMetadata(String metadataLocation) { return TableMetadataParser.read(io(), metadataLocation); }

github-actions bot added build core NESSIE labels Oct 8, 2021

snazy force-pushed the tvs2 branch 2 times, most recently from 9fc9e83 to ae35eef Compare October 9, 2021 11:40

snazy mentioned this pull request Oct 9, 2021

Iceberg integration of Nessie 1.0.0 projectnessie/nessie#2254

Closed

This was referenced Oct 9, 2021

Support viewing snapshot id history for table in branches/tags projectnessie/nessie#2305

Closed

Nessie commit logs from iceberg should have operations info aswell projectnessie/nessie#2281

Closed

snazy force-pushed the tvs2 branch from ae35eef to c341ea6 Compare October 10, 2021 07:50

snazy marked this pull request as draft October 10, 2021 08:56

rdblue reviewed Oct 10, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 10, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

snazy force-pushed the tvs2 branch 3 times, most recently from a636324 to cc353e1 Compare October 13, 2021 13:31

snazy force-pushed the tvs2 branch 5 times, most recently from b22785c to aed18cf Compare October 20, 2021 12:48

snazy changed the title ~~Bump Nessie to 0.10.1 + related changes~~ Bump Nessie to 0.11.0 + related changes Oct 20, 2021

snazy marked this pull request as ready for review October 20, 2021 13:35

snazy force-pushed the tvs2 branch 2 times, most recently from 2b13c6a to 4276e63 Compare October 20, 2021 13:44

ajantha-bhat mentioned this pull request Oct 26, 2021

Update Nessie spec asper #2312 projectnessie/nessie#2501

Closed

harshm-dev reviewed Oct 27, 2021

View reviewed changes

omarsmak reviewed Oct 27, 2021

View reviewed changes

ajantha-bhat mentioned this pull request Oct 29, 2021

Docs: Update spec as per latest on ref state. projectnessie/nessie#2544

Merged

snazy force-pushed the tvs2 branch 2 times, most recently from ff67abc to 486a297 Compare November 4, 2021 09:43

ajantha-bhat mentioned this pull request Nov 9, 2021

Bump Iceberg from 0.12.0 to 0.12.1 projectnessie/nessie#2543

Merged

snazy force-pushed the tvs2 branch from 486a297 to 107c04b Compare November 12, 2021 14:54

snazy changed the title ~~Bump Nessie to 0.12.0 + related changes~~ Bump Nessie to 0.14.0 + related changes Nov 12, 2021

nastra reviewed Nov 17, 2021

View reviewed changes

rymurr requested changes Nov 17, 2021

View reviewed changes

jackye1995 added this to the Iceberg 0.13.0 Release milestone Nov 19, 2021

snazy force-pushed the tvs2 branch 4 times, most recently from 4e62dc1 to 081b130 Compare November 23, 2021 10:06

rymurr reviewed Nov 26, 2021

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/iceberg/TableMetadata.java Show resolved Hide resolved

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java Show resolved Hide resolved

snazy force-pushed the tvs2 branch 2 times, most recently from 9b2de70 to 98ad99a Compare December 1, 2021 14:46

snazy changed the title ~~Bump Nessie to 0.14.0 + related changes~~ Bump Nessie to 0.15.1 + related changes Dec 1, 2021

snazy force-pushed the tvs2 branch 2 times, most recently from d17fe25 to 85f5085 Compare December 1, 2021 16:16

rymurr approved these changes Dec 1, 2021

View reviewed changes

snazy force-pushed the tvs2 branch from 85f5085 to b194c0b Compare December 1, 2021 16:37

ajantha-bhat reviewed Dec 2, 2021

View reviewed changes

site/mkdocs.yml Outdated Show resolved Hide resolved

Update site/mkdocs.yml

169ff47

Co-authored-by: Ajantha Bhat <ajanthabhat@gmail.com>

rymurr merged commit 699bdd1 into apache:master Dec 2, 2021

snazy deleted the tvs2 branch December 2, 2021 18:06

rdblue reviewed Dec 4, 2021

View reviewed changes

Bump Nessie to 0.15.1 + related changes #3257

Bump Nessie to 0.15.1 + related changes #3257

Uh oh!

Conversation

snazy commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snazy commented Oct 10, 2021

Uh oh!

rdblue commented Oct 10, 2021

Uh oh!

Uh oh!

Uh oh!

rdblue commented Oct 10, 2021

Uh oh!

snazy commented Oct 14, 2021

Uh oh!

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

Uh oh!

Uh oh!

snazy commented Oct 23, 2021

Uh oh!

harshm-dev 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!

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

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!

nastra left a comment

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

snazy commented Oct 8, 2021 •

edited

Loading

jackye1995 commented Oct 23, 2021 •

edited

Loading

harshm-dev left a comment •

edited

Loading