HIVE-29405: Decouple the alter table from HMSHandler/HiveAlterHandler - #6674
HIVE-29405: Decouple the alter table from HMSHandler/HiveAlterHandler#6674rtrivedi12 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the HIVE-29402 refactor by extracting alter_table_req handling out of HMSHandler into a dedicated @RequestHandler (AlterTableHandler), aligning alter-table with the request-handler dispatch pattern used elsewhere in metastore-server.
Changes:
- Added
AlterTableHandlerto encapsulate alter-table request preprocessing (envContext shaping, remote DB guard, normalization, transformer + pre-event) and to invokeAlterHandler.alterTable(...). - Updated
HMSHandler.alter_table_reqto delegate execution toAbstractRequestHandler.offer(this, req). - Extended
IHMSHandlerwithgetAlterHandler()and implemented it inBaseHandlerto allow request handlers to access the configured alter handler without reaching intoHMSHandlerinternals.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java | Adds getAlterHandler() to support request-handler execution without direct field access. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java | Delegates alter_table_req to the request-handler framework. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/BaseHandler.java | Implements getAlterHandler() to expose the configured AlterHandler. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/AlterTableHandler.java | New request handler implementing the alter-table flow previously embedded in HMSHandler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| newTable = transformer.transformAlterTable(oldt, newTable, processorCapabilities, processorId); | ||
| } | ||
|
|
||
| ((HMSHandler) handler).firePreEvent(new PreAlterTableEvent(oldt, newTable, handler)); |
| protected AlterTableResult execute() throws TException, IOException { | ||
| handler.getAlterHandler().alterTable(handler.getMS(), handler.getWh(), | ||
| catName, dbname, name, newTable, envContext, handler, validWriteIdList); | ||
| return new AlterTableResult(true); |
There was a problem hiding this comment.
I'm wondering if we can move the alterTable in HiveAlterHandler into this class
There was a problem hiding this comment.
@dengzhhu653 Yes, I think it makes sense to move HiveAlterHandler.alterTable() body into into AlterTableHandler. Will make changes and update the PR .
|
What changes were proposed in this pull request?
Extract alter_table_req from HMSHandler into AlterTableHandler, following the HIVE-29402 @RequestHandler / AbstractRequestHandler pattern.
Why are the changes needed?
Part of HIVE-29402 to split HMSHandler into focused request handlers.
Does this PR introduce any user-facing change?
No
How was this patch tested?
mvn test -pl standalone-metastore/metastore-server
-Dtest=org.apache.hadoop.hive.metastore.TestHiveAlterHandler
-Dtest.groups=org.apache.hadoop.hive.metastore.annotation.MetastoreUnitTest
mvn test -pl standalone-metastore/metastore-server
-Dtest=org.apache.hadoop.hive.metastore.client.TestTablesCreateDropAlterTruncate
-Dtest.groups=org.apache.hadoop.hive.metastore.annotation.MetastoreCheckinTest