Skip to content

HIVE-28930: Implement a metastore service that expires iceberg table snapshots periodically #5786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

HIVE-28930: Implement a metastore service that expires iceberg table snapshots periodically #5786

wants to merge 8 commits into from

Conversation

abstractdog
Copy link
Contributor

@abstractdog abstractdog commented Apr 25, 2025 •
edited
Loading

What changes were proposed in this pull request?

This patch introduces a metastore task as a MetastoreTaskThread that can expire snapshots of iceberg tables periodically according to configuration: catalog name, database pattern, table pattern. The configuration was inspired by the partition management task.

Patch contents:

  1. IcebergHouseKeeperService + TestIcebergHouseKeeperService unit test
  2. added the new task class (ICEBERG_TABLE_SNAPSHOT_EXPIRY_SERVICE_CLASS ) to the default housekeeping threads
  3. MiniHS2 changes: withHouseKeepingThreads (for manual testing)
  4. changing to keepJdbcUri=true in a call, otherwise in remote metastore mode, 2 different derby databases are used, leading to exotic problems
  5. Generalized TableFetcher, which is a basically a table filter builder, originally in the PartitionManagementTask, completely reused + TestTableFetcher unit test

Why are the changes needed?

This service could act as a convenient helper to maintain iceberg tables, which otherwise need explicit hive ql statements by the user.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests added.

Manual testing is also possible, as the patch adds MiniHS2 capability and fixes to run metastore tasks in remote mode, example command:

mvn clean install -Dtest=StartMiniHS2Cluster -DminiHS2.clusterType=llap -DminiHS2.conf="target/testconf/llap/hive-site.xml"  -DminiHS2.run=true -DminiHS2.usePortsFromConf=true -pl itests/hive-unit -Pitests -pl itests/util -DminiHS2.clusterType=LOCALFS_ONLY -DminiHS2.isMetastoreRemote=true -DminiHS2.withHouseKeepingThreads=true

@abstractdog abstractdog mentioned this pull request Apr 25, 2025
Open
@abstractdog abstractdog requested a review from deniskuzZ April 28, 2025 10:24
@abstractdog
Copy link
Contributor Author

@deniskuzZ : this is the reusable, general part of the iceberg table maintenance service (no query history bits can be found here), I would appreciate a review in the future once you have time for that

@deniskuzZ deniskuzZ self-assigned this May 8, 2025
deniskuzZ
deniskuzZ reviewed May 8, 2025
View reviewed changes
Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM, minor comments

okumin
okumin reviewed Jun 2, 2025
View reviewed changes
...tore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
ICEBERG_TABLE_EXPIRY_INTERVAL("metastore.iceberg.table.expiry.interval",
"hive.metastore.iceberg.table.expiry.interval", 3600, TimeUnit.SECONDS,
"Time interval describing how often the iceberg table expiry service runs."),
ICEBERG_TABLE_EXPIRY_CATALOG_NAME("metastore.iceberg.table.expiry.catalog.name",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am, of course.

...tore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java Outdated
"By default, the pattern is set to empty string, which results in no matches (this is intentional" +
"to avoid expensive metastore calls unless explicitly configured by the user)."),
ICEBERG_TABLE_EXPIRY_TABLE_PATTERN("metastore.iceberg.table.expiry.table.pattern",
"hive.metastore.iceberg.table.expiry.table.pattern", "none",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be the empty string, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

...ndler/src/main/java/org/apache/iceberg/mr/hive/metastore/task/IcebergHouseKeeperService.java
if (deleteExecutorService != null) {
expireSnapshots.executeDeleteWith(deleteExecutorService);
}
expireSnapshots.commit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would skip not only ExpireSnapshots#executeDeleteWith but also icebergTable.expireSnapshots() and ExpireSnapshots#commit. That's because the Iceberg library might implement some side effects in the future, especially on commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean something like this?

    if (deleteExecutorService != null) {
      ExpireSnapshots expireSnapshots = icebergTable.expireSnapshots();
      expireSnapshots.executeDeleteWith(deleteExecutorService);
      expireSnapshots.commit();
    }

so, basically, setting hive.iceberg.expire.snapshot.numthreads=0 to completely turn this housekeeper service off?

Copy link
Contributor

@okumin okumin Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I thought executeDeleteWith would perform the side effect. Now, I understand it just configures the executor. We don't need to make any changes. My bad
https://github.com/apache/iceberg/blob/apache-iceberg-1.9.1/api/src/main/java/org/apache/iceberg/ExpireSnapshots.java#L87-L100

...ndler/src/main/java/org/apache/iceberg/mr/hive/metastore/task/IcebergHouseKeeperService.java Outdated
}
} catch (Exception e) {
LOG.error("Exception while running iceberg expiry service on catalog/db/table: {}/{}/{}",
catalogName, dbPattern, tablePattern, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may move the try-catch block within the for-loop because ExpireSnapshots#commit can fail due to a conflict.
https://github.com/apache/iceberg/blob/apache-iceberg-1.9.1/api/src/main/java/org/apache/iceberg/PendingUpdate.java#L43-L55

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, currently, if a table fails to be committed, none of the remaining tables will be attempted, which is not the desired behavior

@abstractdog
Copy link
Contributor Author

thanks again @okumin for the comments, addressed them in 4c5686a

okumin
okumin approved these changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@okumin okumin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@abstractdog
Copy link
Contributor Author

LGTM 👍

thanks @okumin !
I just realized I left a compilation issue :D getTables() can also throw exception, so I changed the outer catch to a more generic one: 18d5358
plese, let me know if you see any problems with that, otherwise I'm assuming that your approval still holds

@okumin
Copy link
Contributor

okumin commented Jun 3, 2025

@abstractdog It sounds fine to me. The build failed. Please take a look and fix it.

[INFO] 878 goals, 878 executed
Error:  Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.44.4:check (validate) on project hive-iceberg-handler: The following files had format violations:
Error:      src/main/java/org/apache/iceberg/mr/hive/metastore/task/IcebergHouseKeeperService.java
Error:          @@ -30,7 +30,6 @@
Error:           import·org.apache.hadoop.hive.metastore.IMetaStoreClient;
Error:           import·org.apache.hadoop.hive.metastore.MetastoreTaskThread;
Error:           import·org.apache.hadoop.hive.metastore.api.GetTableRequest;
Error:          -import·org.apache.hadoop.hive.metastore.api.MetaException;
Error:           import·org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
Error:           import·org.apache.hadoop.hive.metastore.conf.MetastoreConf;
Error:           import·org.apache.hadoop.hive.metastore.txn.NoMutex;
Error:  Run 'mvn spotless:apply' to fix these violations.
Error:  -> [Help 1]

@abstractdog
Copy link
Contributor Author

@abstractdog It sounds fine to me. The build failed. Please take a look and fix it.

[INFO] 878 goals, 878 executed
Error:  Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.44.4:check (validate) on project hive-iceberg-handler: The following files had format violations:
Error:      src/main/java/org/apache/iceberg/mr/hive/metastore/task/IcebergHouseKeeperService.java
Error:          @@ -30,7 +30,6 @@
Error:           import·org.apache.hadoop.hive.metastore.IMetaStoreClient;
Error:           import·org.apache.hadoop.hive.metastore.MetastoreTaskThread;
Error:           import·org.apache.hadoop.hive.metastore.api.GetTableRequest;
Error:          -import·org.apache.hadoop.hive.metastore.api.MetaException;
Error:           import·org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
Error:           import·org.apache.hadoop.hive.metastore.conf.MetastoreConf;
Error:           import·org.apache.hadoop.hive.metastore.txn.NoMutex;
Error:  Run 'mvn spotless:apply' to fix these violations.
Error:  -> [Help 1]

eh, sorry, committing one more

okumin
okumin approved these changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@okumin okumin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me if CI is successful

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jun 3, 2025

Quality Gate Passed Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deniskuzZ deniskuzZ deniskuzZ left review comments

@okumin okumin okumin approved these changes

Assignees

@deniskuzZ deniskuzZ

Labels
tests failed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abstractdog @okumin @deniskuzZ @asf-ci-hive