Bump Nessie to 0.28.0, improve commit conflict detection #4594

snazy · 2022-04-20T11:49:37Z

Enhances Iceberg commit conflict detection by maintaining the commit-id from which a
table metadata has been loaded, to use it as the "expected hash" in a Nessie commit.

Makes Nessie specific properties available in TableMetadata properties:

nessie.content.id - the Nessie Content.getId()
nessie.commit.id - the commit ID used to retrieve the table metadata
nessie.reference.name - the reference name from which the table metadata has been loaded

Also fixes an issue via org.apache.iceberg.nessie.NessieTableOperations#loadTableMetadata
that caused too many "previous files", because the TableMetadata.buildFrom() assumed that
it's only called for ongoing modifications.

nastra · 2022-04-20T13:32:26Z

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

          "is no longer valid.", client.getRef().getName()), e);
    }
    String metadataLocation = null;
+    Reference reference = client.getRef().getReference();


I'm assuming these changes were done to not do additional API calls. These changes won't be necessary after #4593

Calling Supplier.get() a couple of times isn't necessary either.

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

nastra · 2022-04-20T14:17:07Z

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


      this.snapshotLog = Lists.newArrayList(base.snapshotLog);
-      this.previousFileLocation = base.metadataFileLocation;
+      this.previousFileLocation = noPreviousFile ? null : base.metadataFileLocation;


a little bit annoying that this needs to be set, so I wonder whether it would be better to have a builder method that allows overriding this

I need to override that too for something I'm working on. But I have just used the builder's withMetadataLocation. In my case, the value of base.metadataFIleLocation is already null so just calling the function works fine.

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

nastra · 2022-04-20T14:25:02Z

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

    } catch (NessieNotFoundException ex) {
      throw new RuntimeException(
-          String.format("Cannot commit: Reference '%s' no longer exists", client.getRef().getName()), ex);
+          String.format("Cannot commit: Reference '%s' no longer exists", updateableReference.getName()), ex);


nit: I would probably revert all of those changes as I don't think they are necessary

Calling Supplier.get() a couple of times isn't necessary either.

kbendick · 2022-04-21T16:53:23Z

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

+    TableMetadata.Builder builder = TableMetadata.buildFrom(deserialized, true)
        .setCurrentSchema(table.getSchemaId())
        .setDefaultSortOrder(table.getSortOrderId())
        .setDefaultPartitionSpec(table.getSpecId())
-        .withMetadataLocation(metadataLocation);
+        .withMetadataLocation(metadataLocation)


I think you don't need to pass true in here. If base doesn't have that field set, it will just be null. Otherwise it will get overridden after this builder is called with .build().

The problem here is that the buildFrom/Builder.<init>() sets the final field previousFileLocation, which is then used in Builder.build() to add a MetadataLogEntry. Builder.previousFileLocation and Builder.metadataLocation are different.

RussellSpitzer · 2022-04-26T14:31:04Z

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

    }
  }

-  private static void toJson(TableMetadata metadata, JsonGenerator generator) throws IOException {


I don't think we want to change accessibility on these methods, why is the custom JsonGenerator needed in the Nessie code?

sigh
fair enough

RussellSpitzer · 2022-04-26T14:36:18Z

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

-    TableMetadata.Builder builder = TableMetadata.buildFrom(TableMetadataParser.read(io(), metadataLocation))
+    TableMetadata deserialized;
+    if (table.getMetadata() != null) {
+      deserialized = TableMetadataParser.fromJson(io(), metadataLocation, table.getMetadata().getMetadata());


So we have this path in to request metadata out of a Nessie specific object which stores the metadata as a Json String that needs to be parsed?

I think i see, the IcebergTable class has a JsonNode? And our current public method only accepts a string, it doesn't allow directly injecting the node. I kind of think we should probably stick to that "string" api rather that opening up another method to the public but I may be paranoid.

(Not yet, but in the future, yes.)

It's already a JSON node tree (as a generic JsonNode) - so the text->nodes parsing already happened.
As I didn't want to serialize the JsonNode to a String to get a TableMetadata, I've updated the TableMetadataParser.

RussellSpitzer · 2022-04-26T14:36:32Z

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

+    if (table.getMetadata() != null) {
+      deserialized = TableMetadataParser.fromJson(io(), metadataLocation, table.getMetadata().getMetadata());
+    } else {
+      deserialized = TableMetadataParser.read(io(), metadataLocation);


Otherwise we just read from the location on disk?

RussellSpitzer · 2022-04-26T14:41:16Z

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

    return new Builder(base);
  }

+  public static Builder buildFrom(TableMetadata base, boolean noPreviousFile) {


A new public api here needs a java doc, I'm not sure I follow what noPreviousFile is expected to do here

That's a "smart thing" in the builder. If it's present, then it add it as a history item. That behavior is okay, when buildFrom() is only used when it is used within a "modify TableMetadata operation". But the Nessie catalog only uses it to modify the TableMetadata as it is - so it's not a change, which requires a new history-item.
Without the noPreviousFile==true, a couple of catalog tests fail, because those assert on the number of history items.

@snazy I've been thinking on this and I'm wondering why can't implement a builder like .withPreviousFileLocation to set that to previousFileLocation to null

WFM, updated the PR

RussellSpitzer · 2022-04-26T14:46:20Z

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

+      JsonNode newMetadata;
+      try {
+        TokenBuffer tokenBuffer = new TokenBuffer(JsonUtil.factory().getCodec(), false);
+        TableMetadataParser.toJson(metadata, tokenBuffer);


So we could avoid this usage if we use the "toJson(metadata) method and then pass that into the token buffer. I know it's a bit more wasteful but we are only doing this once per operation and it let's us keep the toJson(metadata, generator) method package private. Again I may be being paranoid here but ideally we don't open up any more methods than we absolutely have to.

Enhances Iceberg commit conflict detection by maintaining the commit-id from which a table metadata has been loaded, to use it as the "expected hash" in a Nessie commit. Makes Nessie specific properties available in `TableMetadata` properties: * `nessie.content.id` - the Nessie `Content.getId()` * `nessie.commit.id` - the commit ID used to retrieve the table metadata * `nessie.reference.name` - the reference name from which the table metadata has been loaded Also fixes an issue via `org.apache.iceberg.nessie.NessieTableOperations#loadTableMetadata` that caused too many "previous files", because the `TableMetadata.buildFrom()` assumed that it's only called for ongoing modifications.

RussellSpitzer · 2022-05-04T16:12:58Z

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

    } catch (NessieNotFoundException ex) {
      throw new RuntimeException(
-          String.format("Cannot commit: Reference '%s' no longer exists", client.getRef().getName()), ex);
+          String.format("Cannot commit: Reference '%s' no longer exists",


Just a formatting change

RussellSpitzer · 2022-05-04T16:13:08Z

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

  private String buildCommitMsg(TableMetadata base, TableMetadata metadata) {
    if (isSnapshotOperation(base, metadata)) {
-      return String.format("Iceberg %s against %s", metadata.currentSnapshot().operation(), tableName());
+      return String.format("Iceberg %s against %s", metadata.currentSnapshot().operation(),


Just a formatting change

github-actions bot added build core NESSIE labels Apr 20, 2022

nastra reviewed Apr 20, 2022

View reviewed changes

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

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

nastra reviewed Apr 20, 2022

View reviewed changes

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

nastra reviewed Apr 20, 2022

View reviewed changes

snazy force-pushed the nessie-0.27.0 branch 2 times, most recently from b4c5b88 to 0fd33b4 Compare April 21, 2022 08:50

kbendick reviewed Apr 21, 2022

View reviewed changes

nastra requested a review from RussellSpitzer April 22, 2022 06:58

nastra mentioned this pull request Apr 22, 2022

Nessie: Support Namespace properties #4610

Merged

snazy force-pushed the nessie-0.27.0 branch 2 times, most recently from d36118d to bfc6655 Compare April 26, 2022 13:29

snazy changed the title ~~Bump Nessie to 0.27.0, improve commit conflict detection~~ Bump Nessie to 0.28.0, improve commit conflict detection Apr 26, 2022

snazy requested a review from nastra April 26, 2022 13:30

RussellSpitzer reviewed Apr 26, 2022

View reviewed changes

snazy force-pushed the nessie-0.27.0 branch from bfc6655 to 55c3505 Compare April 26, 2022 16:10

snazy force-pushed the nessie-0.27.0 branch from 55c3505 to fffb89f Compare May 2, 2022 12:28

RussellSpitzer reviewed May 4, 2022

View reviewed changes

RussellSpitzer approved these changes May 4, 2022

View reviewed changes

RussellSpitzer merged commit eb1fd41 into apache:master May 4, 2022

snazy deleted the nessie-0.27.0 branch May 5, 2022 09:46

Bump Nessie to 0.28.0, improve commit conflict detection #4594

Bump Nessie to 0.28.0, improve commit conflict detection #4594

Uh oh!

Conversation

snazy commented Apr 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snazy Apr 20, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snazy Apr 20, 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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snazy Apr 20, 2022 •

edited

Loading

snazy Apr 20, 2022 •

edited

Loading