HIVE-29436: Iceberg materialized view rebuild locking issue#6330
HIVE-29436: Iceberg materialized view rebuild locking issue#6330kasakrisz wants to merge 3 commits intoapache:masterfrom
Conversation
| alter materialized view mat1 rebuild; | ||
|
|
||
| alter materialized view mat1 rebuild; | ||
| alter materialized view mat2 rebuild; |
There was a problem hiding this comment.
In this test we are checking if rebuild for different materialized views succeeds. Can we also test if rebuilds of the same MV also works both in a serialized way within a session and in a concurrent sessions scenario ?
|
|
||
| if (!this.ctx.isExplainPlan() && (AcidUtils.isTransactionalTable(table) || | ||
| table.isNonNative() && table.getStorageHandler().areSnapshotsSupported())) { | ||
| if (!this.ctx.isExplainPlan() && (AcidUtils.isTransactionalTable(table))) { |
There was a problem hiding this comment.
When the materialized view is stored in Hive ACID with source table as an iceberg, it would still fail. Mentioned in https://issues.apache.org/jira/browse/HIVE-29496. Can close it as duplicate once that is fixed too.
kasakrisz
force-pushed
the
HIVE-29436-master-iceberg-mv-lock
branch
from
0a3228e to
fea89fb
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
kasakrisz
force-pushed
the
HIVE-29436-master-iceberg-mv-lock
branch
from
fea89fb to
8a458be
Compare
|
| rewrittenAST = ParseUtils.parse(rewrittenInsertStatement, ctx); | ||
| this.ctx.addSubContext(ctx); | ||
|
|
||
| if (!this.ctx.isExplainPlan() && (AcidUtils.isTransactionalTable(table) || |
There was a problem hiding this comment.
is it safe to drop? it covers not only iceberg tables
| HiveTxnManager txnManager = getTxnMgr(); | ||
| LockState state; | ||
| try { | ||
| state = txnManager.acquireMaterializationRebuildLock( |
There was a problem hiding this comment.
in case it's not needed, why not drop the API as well?
There was a problem hiding this comment.
I found that there is a TxnType.MATER_VIEW_REBUILD so which I guess could be used instead of the acquireMaterializationRebuildLock functionality. I would like to add a test to confirm that it is safe to drop the latter.
The test should run two rebuilds parallel targeting the same MV:
-- Session #1
start transaction
alter materialized view mat1 rebuild;
-- Session #2
start transaction
alter materialized view mat1 rebuild;
commit;
-- Session #1
commit; -- this commit should fail or rollback
Do you know if there are any tests for running DDLs on the same table ?
There was a problem hiding this comment.
DDL is not supported inside transaction block, only DML. Note, I haven't tested this, it might work, but i doubt that.
please check the HiveIcebergOutputCommitter
Transaction txn = IcebergAcidUtil.getOrCreateTransaction(table, jobConf)
something similar probably would be needed in HiveIcebergMetaHook.
Maybe you could use EXCL_WRITE lock instead to serialize the MV rebuilds
There was a problem hiding this comment.
Sorry I miss typed it. I meant DML.
I would like to test both ACID and Iceberg tables/MV but it is hard to create 2 sessions from test. It is not possible with q test. I'm trying to use MiniHS2 via jdbc but it seems that even if I start a transaction the insert into an ACID table statement commits the txn automatically
statement.execute("start transaction");
statement.execute("insert into t1 values (0, 1), (2, 20)");
statement.execute("commit");
throws:
org.apache.hive.service.cli.HiveSQLException: Error while compiling statement: FAILED: LockException [Error 20008]: Operation COMMIT is not allowed without an active
There was a problem hiding this comment.
check the iceberg_multi_table_txn.q or TestConcurrentMultiTableTransactions
Note, start transaction is under the feature flag ATM, you need to set "hive.in.test"=true
5 participants
What changes were proposed in this pull request?
Do not acquire a rebuild lock for materialized views stored by Iceberg because we rely on Iceberg's concurrency management.
Why are the changes needed?
Subsequent materialized view rebuilds may fail. See jira for details: https://issues.apache.org/jira/browse/HIVE-29436
Does this PR introduce any user-facing change?
No.
How was this patch tested?