HIVE-28793: Expire query history snapshots #5666
Conversation
There was a problem hiding this comment.
Thanx @abstractdog will it be possible to extend a test for it as well
|
|
||
| private void expireSnapshots() { | ||
| storageHandler.executeOperation(table, new AlterTableExecuteSpec<>(EXPIRE_SNAPSHOT, | ||
| new AlterTableExecuteSpec.ExpireSnapshotsSpec(10))); |
There was a problem hiding this comment.
Do you want to hardcode 10 here or just pass null & let the TBLPROPERTIES handle it, this way it can be configurable also by ALTER command.
I don't think this would work the way you expect, there is some default time thing as well
public static final String MAX_SNAPSHOT_AGE_MS = "history.expire.max-snapshot-age-ms";
public static final long MAX_SNAPSHOT_AGE_MS_DEFAULT = 432000000L;
There was a problem hiding this comment.
thanks @ayushtkn for taking a look!
good point, I'm not familiar with EXPIRE SNAPSHOT functionality detail, so this is more like a DRAFT, let me consider
There was a problem hiding this comment.
this seemed to work:
4c9480f
added testing details in PR description
There was a problem hiding this comment.
@ayushtkn : I just added a unit test for snapshot expiry testing
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
|
while testing this patch, I realized that the meta hook commit process doesn't respect hiveLockEnabled, modified it, quite a small change but not related query history, please let me know if it can stay here @ayushtkn , @deniskuzZ |
|
IMO we should split it out into a separate ticket, if it isn't related to QueryHistory |
ack, just created #5687 |
|
unit test passed without the locking hack (most probably it was an incomplete step that I worked around with proper configuration in the meantime) |
|
| Table table = initTable(hive, database); | ||
| postInitTable(table); | ||
| initTable(hive, database); | ||
| postInitTable(); |
There was a problem hiding this comment.
could we trigger postInitTable within the initTable?
| @@ -176,4 +193,13 @@ private void prepareConfForWrite() { | |||
| SessionStateUtil.addCommitInfo(SessionState.getSessionConf(), tableDesc.getTableName(), jobId, 1, | |||
| Maps.fromProperties(tableDesc.getProperties())); | |||
| } | |||
|
|
|||
| private void expireSnapshots() { | |||
There was a problem hiding this comment.
We have an AcidHouseKeeperService, I think we should create a similar IcebergHouseKeeperService and schedule periodic snapshot expiry on Iceberg tables.
You can define the expiry policy for the query-history table and IcebergHouseKeeperService / IcebergTableExpiryService would take care of it
There was a problem hiding this comment.
yeah, theoretically, handling them in a centralized place in metastore instead of multiple HS2s looks better from system design point of view
however, it needs a new service to be implemented, let me check
There was a problem hiding this comment.
no need for a "NEW" service, add it to the list of TASK_THREADS_REMOTE_ONLY
but yes, that service should iterate over all iceberg tables with expiry configs and periodically trigger the snapshot expiry action
take a look at AcidTxnCleanerService
public long runFrequency(TimeUnit unit) {
return INTERVAL;
}
public void run() {
doTheMagic()
}
There was a problem hiding this comment.
took a look, my concern about this whole service is exactly the doTheMagic part: I remember we faced lots of problems with e.g. the PartitionManagementTask because it couldn't have been run in a lightweight fashion, and the common thing is that we need to iterate on all tables of all DBs to achieve what we want here: the best case scenario is that we implement a filter on ICEBERG tables (like: listTableNamesByFilter), kinda pushing down the filter to backend db
but even if we do that, we still need lots of code to make it as optimal as possible, just take a look at how complicated the partition management task filtering logic is to achieve the same).
So looking at the implementations so far, the "getting all iceberg tables" is the very part I'm worried about, and makes me wonder I really want to go towards that direction just the sake of this query history snapshot expiry, which is otherwise a very simple code
Focusing on the motivation, which is to handle the query history table—fully loaded and maintained by Hive—we owe it to users to optimize it as much as possible. However, the question is whether we should take on the additional burden in this PR to handle all Iceberg tables visible in the metastore, or keep the scope focused for now.
Or maybe a similar db + table name filter (that's used in the part mgmt task), but we default it to: "sys" + "query_history" in MetastoreConf and give the user the power to set it even to * + * ?
There was a problem hiding this comment.
- you don't need to go down to partition level;
- you can get list of iceberg tables from HMS, there is an API for that;
- yes, service should be optimized.
- check the last housekeeping event,
- check if there were any changes to the table since last attempt (check if metadata file hasn't changed),
- support table name filtering;
- adding it just for query history, doesn't bring much value and is not generic;
- downstream you can configure all policies in DLM so you don't need this at all;
ql/src/java/org/apache/hadoop/hive/ql/queryhistory/repository/IcebergRepository.java
Show resolved
Hide resolved
What changes were proposed in this pull request?
Introduce a periodic snapshot expiry for query history iceberg table.
Why are the changes needed?
To expire snapshots to delete data files that are no longer needed, and to reduce the size of table metadata. Each write to an Iceberg table from Hive creates a new snapshot, or version, of a table. Snapshots can be used for time-travel queries, or for rollbacks, but for query history table in particular, that use-case is not typical.
Does this PR introduce any user-facing change?
No.
Is the change a dependency upgrade?
No.
How was this patch tested?
Local MiniHS2 with low expiry time and checked periodicity and filesystem.
and in filesystem: