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
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
|
@zhangbutao need a review here. Thanks |
@Neer393 Thanks for pinging me. I am on vacation these days. I will review the code once I have time. |
...e-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/LockComponent.java
Outdated
Show resolved
Hide resolved
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
soumyakanti3578
left a comment
soumyakanti3578
left a comment
There was a problem hiding this comment.
I have reviewed parts of the PR and will get back to it later. It would be nice to go through the sonar issues and fix the ones introduced by this PR. That will increase maintainability of the code in the future.
...e-unit/src/test/java/org/apache/hadoop/hive/ql/parse/BaseReplicationScenariosAcidTables.java
Outdated
Show resolved
Hide resolved
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/MetastoreLock.java
Outdated
Show resolved
Hide resolved
|
|
||
| LockResponse get_lock_materialization_rebuild(1: string dbName, 2: string tableName, 3: i64 txnId) | ||
| bool heartbeat_lock_materialization_rebuild(1: string dbName, 2: string tableName, 3: i64 txnId) | ||
| LockResponse get_lock_materialization_rebuild(1: string catName, 2: string dbName, 3: string tableName, 4: i64 txnId) |
There was a problem hiding this comment.
This is a compatibility-breaking change. If we can confirm that this method is only used internally by Hive, then this change is safe. However, if other open-source components also call this method, we need to consider retaining the original method and adding a new one instead.
Based on my current understanding, this method is only used internally by Hive. I’d like to ping other folks to take a look together. cc @dengzhhu653 @deniskuzZ
There was a problem hiding this comment.
I have made changes as asked and also added a unit test and a qtest with different catalog.
I have addressed all comments and fixed the indentation issues raised by sonar qube as well.
Only this comment is to be validated.
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
Outdated
Show resolved
Hide resolved
...e-unit/src/test/java/org/apache/hadoop/hive/ql/parse/BaseReplicationScenariosAcidTables.java
Outdated
Show resolved
Hide resolved
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksAnalyzer.java
Show resolved
Hide resolved
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksAnalyzer.java
Outdated
Show resolved
Hide resolved
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksAnalyzer.java
Outdated
Show resolved
Hide resolved
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksOperation.java
Outdated
Show resolved
Hide resolved
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
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;