Hive: Refactor hive-table commit operation to be used for other operations like view by nk1506 · Pull Request #9461 · apache/iceberg

nk1506 · 2024-01-12T07:07:51Z

nk1506 · 2024-01-12T07:08:31Z

nk1506 · 2024-01-12T07:18:07Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java


-    } finally {
-      cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+    if (!conf.getBoolean(ConfigProperties.KEEP_HIVE_STATS, false)) {


All the obsolete props are being merged together now. Earlier It was being managed by
removedProps
and hiveStatCheck

nk1506 · 2024-01-12T07:19:43Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java

  }
+
+  @SuppressWarnings("checkstyle:CyclomaticComplexity")
+  default void commitWithLocking(


Idea here is to generalise doCommit from HiveTableOperations. It should be used by view too.

nk1506 · 2024-01-17T08:22:10Z

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

   * @return Commit Status of Success, Failure or Unknown
   */
  protected CommitStatus checkCommitStatus(String newMetadataLocation, TableMetadata config) {
-    int maxAttempts =


This needed to move to a common place so that other operations like View can also use it.

nk1506 · 2024-01-23T06:45:29Z

@szehon-ho With issue #9514 we are discussing what would be the best approach to handle this? We have not concluded on anything yet which is why this PR is still in progress state.

szehon-ho

Took a first pass

szehon-ho · 2024-01-25T21:58:17Z

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

-          maxAttempts);
-    }
-    return status.get();
+    return MetastoreOperationsUtil.checkCommitStatus(


My question here is, does it have to be a Util?

Usually in Iceberg, I notice that we tend to do shared code using abstract classes. For example, a BaseMetastoreOperations that defines the common methods, and then have BaseMetastoreTableOperations and then BaseMetastoreViewOperations. That way, we can make those be 'protected'

szehon-ho · 2024-01-25T23:40:40Z

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

+      String tableName,
+      String newMetadataLocation,
+      Map<String, String> properties,
+      Function<String, Boolean> commitStatusSupplier) {


How about, passing in a method to just get the current metadata location(s)? ie, Runnable<List> refreshMetadataLocations

Then the common logic can include the comparison with new metadata location?

I couldn't understand this part very well. Could you please elaborate more?
The validation happens either with current metadata location or with previous files. How only current metadata location is going to help?

I meant something like:

checkCommitStatus(String newMetadataLocation, TableMetadata config, Runnable<List<String>> loadMetadataLocations) ... // existing common code Tasks.foreach(newMetadataLocation) .retry(maxAttempts) .suppressFailureWhenFinished() .exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0) .onFailure( (location, checkException) -> LOG.error("Cannot check if commit to {} exists.", tableName(), checkException)) .run( location -> { List<String> allMetadataLocations = loadMetadataLocations(); boolean commitSuccess = allMetadataLocations.contains(newMetadataLocations); if (commitSuccess) { ... status.set(CommitStatus.SUCCESS); } else { ... } }); ... // rest of existing common code

That way, we can just have the different implementations like:

HiveTableOperations { List<String> loadMetadataLocations() { TableMetadata metadata = refresh(); return ImmutableList.Builder .add(metadata.metadataFileLocation()) .addAll(metadata.previousFiles()) .build(); } } ViewTableOperations { List<String> loadMetadataLocations() { ViewMetadata metadata = refresh(); return ImmutableList.Builder .add(metadata.metadataFileLocation()) .addAll(metadata.previousFiles()) .build(); } }

and pass this::loadMetadataLocations() into checkCommitStatus() call. Is that clearer? This allows us to more code, if we dont go with the common interface for View and Table hierarchies.

Do you mean runnable or supplier here for lazy loading ?

szehon-ho

Hi , I am sorry for the late reply.

I left some more comments and explanations of preivous comments.

I am also now thinking, whether it makes sense to have a common interface for TableMetadata and ViewMetadata, as @pvary originally suggested. Something like HasLocations or HasProperties may help us already remove lots of duplicate code here. There is already precdent in Table => HasTableOperations, and many other interfaces in Iceberg code, and should not be very visible to the user.

szehon-ho · 2024-02-06T01:48:24Z

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

+  /**
+   * Attempt to load the table and see if any current or past metadata location matches the one we
+   * were attempting to set. This is used as a last resort when we are dealing with exceptions that
+   * may indicate the commit has failed but are not proof that this is the case. Past locations must


Nit: 'are not proof' => 'don't have proof'

How about 'Past locations must ...' => 'Note that all previous locations must ...'

szehon-ho · 2024-02-06T01:52:31Z

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

+            COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
+
+    AtomicReference<BaseMetastoreTableOperations.CommitStatus> status =
+        new AtomicReference<>(BaseMetastoreTableOperations.CommitStatus.UNKNOWN);


Can we move CommitStatus to this class, and make it protected?

That way the BaseMetastoreTableOperations should not need to make it public.

We can. with the following options:

Move CommitStatus from BaseMetastoreTableOperations to BaseMetastoreOperations which will show changes to many files related to other catalogs.

Keep CommitStatus for both BaseMetastoreTableOperations and BaseMetastoreOperations

Please suggest .

szehon-ho · 2024-02-06T02:00:52Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

-    CommitStatus commitStatus = CommitStatus.FAILURE;
-    boolean updateHiveTable = false;
-
    HiveLock lock = lockObject(metadata);


Is it possible to put

try { HiveLock lock = lockObject(); ... finally { lock.unlock() }

in the common method as well?

HiveLock is not exposed as public.

I though to keep lock and unlock operations with doCommit.

szehon-ho · 2024-02-06T02:12:08Z

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

+      String tableName,
+      String newMetadataLocation,
+      Map<String, String> properties,
+      Function<String, Boolean> commitStatusSupplier) {


I meant something like:

checkCommitStatus(String newMetadataLocation, TableMetadata config, Runnable<List<String>> loadMetadataLocations) ... // existing common code Tasks.foreach(newMetadataLocation) .retry(maxAttempts) .suppressFailureWhenFinished() .exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0) .onFailure( (location, checkException) -> LOG.error("Cannot check if commit to {} exists.", tableName(), checkException)) .run( location -> { List<String> allMetadataLocations = loadMetadataLocations(); boolean commitSuccess = allMetadataLocations.contains(newMetadataLocations); if (commitSuccess) { ... status.set(CommitStatus.SUCCESS); } else { ... } }); ... // rest of existing common code

That way, we can just have the different implementations like:

HiveTableOperations { List<String> loadMetadataLocations() { TableMetadata metadata = refresh(); return ImmutableList.Builder .add(metadata.metadataFileLocation()) .addAll(metadata.previousFiles()) .build(); } } ViewTableOperations { List<String> loadMetadataLocations() { ViewMetadata metadata = refresh(); return ImmutableList.Builder .add(metadata.metadataFileLocation()) .addAll(metadata.previousFiles()) .build(); } }

and pass this::loadMetadataLocations() into checkCommitStatus() call. Is that clearer? This allows us to more code, if we dont go with the common interface for View and Table hierarchies.

nk1506 · 2024-02-06T12:17:59Z

Hi , I am sorry for the late reply.

I left some more comments and explanations of preivous comments.

I am also now thinking, whether it makes sense to have a common interface for TableMetadata and ViewMetadata, as @pvary originally suggested. Something like HasLocations or HasProperties may help us already remove lots of duplicate code here. There is already precdent in Table => HasTableOperations, and many other interfaces in Iceberg code, and should not be very visible to the user.

We already have a thread for the same #9514.

…tions like view.

github-actions · 2024-10-13T00:16:24Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2024-10-20T00:16:57Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot added core hive labels Jan 12, 2024

nk1506 commented Jan 12, 2024

View reviewed changes

nk1506 commented Jan 17, 2024

View reviewed changes

szehon-ho mentioned this pull request Jan 23, 2024

Hive: Add View support for HIVE catalog #8907

Closed

szehon-ho reviewed Jan 25, 2024

View reviewed changes

nk1506 force-pushed the hive_table_ops_refactor branch 2 times, most recently from 67077a8 to d3e1555 Compare January 27, 2024 01:57

nk1506 requested a review from szehon-ho February 2, 2024 01:17

szehon-ho reviewed Feb 6, 2024

View reviewed changes

nk1506 mentioned this pull request Feb 6, 2024

Enhancement of ViewMetadata #9514

Closed

nk1506 force-pushed the hive_table_ops_refactor branch 4 times, most recently from 631d867 to 6788222 Compare February 7, 2024 15:54

This was referenced Feb 8, 2024

[WIP] abstraction for TableMetadata and ViewMetadata nk1506/iceberg#9

Closed

Core: Common metadata for TableMetadata and ViewMetadata #9682

Closed

Hive: Refactor hive-table commit operation to be used for other opera…

2fa945c

…tions like view.

nk1506 force-pushed the hive_table_ops_refactor branch from 6788222 to 2fa945c Compare February 14, 2024 08:23

nk1506 mentioned this pull request Mar 2, 2024

Hive: Add View support for HIVE catalog #9852

Merged

github-actions bot added the stale label Oct 13, 2024

github-actions bot closed this Oct 20, 2024

Conversation

nk1506 commented Jan 12, 2024

Uh oh!

nk1506 commented Jan 12, 2024

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!

nk1506 commented Jan 23, 2024

Uh oh!

szehon-ho 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nk1506 Feb 7, 2024 • 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!

nk1506 commented Feb 6, 2024

Uh oh!

github-actions bot commented Oct 13, 2024

Uh oh!

github-actions bot commented Oct 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nk1506 Feb 7, 2024 •

edited

Loading