HIVE-29244: Add catalog field into ShowLocksRequest#6314
HIVE-29244: Add catalog field into ShowLocksRequest#6314Neer393 wants to merge 1 commit intoapache:masterfrom
Conversation
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 84 out of 84 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-4.2.0-to-4.3.0.mysql.sql
Show resolved
Hide resolved
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-4.2.0-to-4.3.0.oracle.sql
Show resolved
Hide resolved
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-4.2.0-to-4.3.0.mssql.sql
Show resolved
Hide resolved
standalone-metastore/metastore-server/src/main/sql/hive/upgrade-4.2.0-to-4.3.0.hive.sql
Show resolved
Hide resolved
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-4.2.0-to-4.3.0.postgres.sql
Show resolved
Hide resolved
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-4.2.0-to-4.3.0.derby.sql
Show resolved
Hide resolved
streaming/src/java/org/apache/hive/streaming/TransactionBatch.java
Outdated
Show resolved
Hide resolved
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/Msck.java
Outdated
Show resolved
Hide resolved
...ore/metastore-common/src/main/protobuf/org/apache/hadoop/hive/metastore/hive_metastore.proto
Show resolved
Hide resolved
asf-ci-hive
added
tests pending
tests failed
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests failed
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java
Outdated
Show resolved
Hide resolved
|
|
||
| components.add(createLockComponent(LockType.SHARED_WRITE, LockLevel.DB, "mydb", "mytable", null, DataOperationType.UPDATE)); | ||
| components.add(createLockComponent(LockType.SHARED_WRITE, LockLevel.DB, "mydb", "yourtable", "mypartition=myvalue", DataOperationType.UPDATE)); | ||
| components.add(createLockComponent(LockType.SHARED_WRITE, LockLevel.DB, "hive", "mydb", |
There was a problem hiding this comment.
what is the point doing doing changes here. can't you just use hive hardcode in createLockComponent?
There was a problem hiding this comment.
Yeah I did think of it but say in future new tests added to this file wants to create a lock component on some table that is not in hive catalog some new catalog. In that case this method won't be usable. Just for extensibility sake I have modified the usages.
If you want, I can hardcode hive in createLockComponent method itself.
ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestDeltaFilesMetrics.java
Show resolved
Hide resolved
...astore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
Outdated
Show resolved
Hide resolved
...astore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
Outdated
Show resolved
Hide resolved
...e/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/entities/LockInfo.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/hadoop/hive/metastore/txn/jdbc/functions/OnRenameFunction.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/hadoop/hive/metastore/txn/jdbc/functions/OnRenameFunction.java
Outdated
Show resolved
Hide resolved
| @@ -1,3 +1,5 @@ | |||
| SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0'; | |||
|
|
|||
| ALTER TABLE `HIVE_LOCKS` ADD COLUMNS (`HL_CATALOG` string); | |||
There was a problem hiding this comment.
i don't think it's a good idea to use full catalog name here instead of FK reference
There was a problem hiding this comment.
Yes I did think of same but then I saw the current implementation like we could have done the same i.e create FK reference for db and table as well but I see the current implementation stores the names and not reference as string so I decided to stick with it.
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
|
Changes have been made as per the comments. |
zhangbutao
left a comment
zhangbutao
left a comment
There was a problem hiding this comment.
@Neer393 Change looks good to me. Thanks @Neer393 .Waiting for @deniskuzZ to take a look.
ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java
Outdated
Show resolved
Hide resolved
...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
Show resolved
Hide resolved
...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 82 out of 85 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Show resolved
Hide resolved
|
What changes were proposed in this pull request?
Modified SHowLocksRequest and ShowLocksResponse to include the catalog name and modified the grammar to accept catalog name for getting locks. Added a catalog field to the HMS HIVE_LOCKS table for 4.3.0 and added an alter statement for upgrading from 4.2.0 to 4.3.0
Why are the changes needed?
The changes are needed so that now user can view locks on tables and databases across catalogs.
Does this PR introduce any user-facing change?
No user facing change
How was this patch tested?
Checked the HIVE_LOCKS table and it had the Catalog column and also tried
SHOW LOCKS DATABASE dbname;which displayed the catalog column as well. Also tried outSHOW LOCKS DATABASE catname.dbname;