You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "ngsg (via GitHub)" <gi...@apache.org> on 2025/07/11 01:50:13 UTC

[PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

ngsg opened a new pull request, #5955:
URL: https://github.com/apache/hive/pull/5955

   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
     2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
     3. Ensure you have added or run the appropriate tests for your PR: 
     4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX:  Your PR title ...'.
     5. Be sure to keep the PR description updated to reflect all changes.
     6. Please write your PR title to summarize what this PR proposes.
     7. If possible, provide a concise example to reproduce the issue for a faster review.
   
   -->
   
   ### What changes were proposed in this pull request?
   <!--
   Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. 
   If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
     1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
     2. If you fix some SQL features, you can provide some references of other DBMSes.
     3. If there is design documentation, please add the link.
     4. If there is a discussion in the mailing list, please add the link.
   -->
   This patch aims to support configurable IMetaStoreClient.
   
   This patch has been submitted three times previously. Austin originally proposed HIVE-12679 and submitted the initial patch in 2016. @moomindani later resubmitted it in https://github.com/apache/hive/pull/1402, and then @okumin took it over in https://github.com/apache/hive/pull/4444.
   
   ### Why are the changes needed?
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you propose a new API, clarify the use case for a new API.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   In some scenarios, users may want to run Hive as a general-purpose execution engine on top of a third-party data catalog. For instance, [AWS Glue Data Catalog](https://github.com/awslabs/aws-glue-data-catalog-client-for-apache-hive-metastore) is an example of such usage, and there is also an open ticket to implement an Iceberg REST catalog (HIVE-28658). As noted in the previous PR (https://github.com/apache/hive/pull/4444), more than 40 people are watching HIVE-12679, which I believe reflects strong demand for using Hive in this more flexible architecture. Therefore, I believe it is worth completing this patch to support external data catalogs in Hive.
   
   ### Does this PR introduce _any_ user-facing change?
   <!--
   Note that it means *any* user-facing change including all aspects such as the documentation fix.
   If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
   If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
   If no, write 'No'.
   -->
   No, users can use the existing HMS client without any changes in their configuration files. 
   For the documentation fix, I will update hive-site once this patch is accepted and merged.
   
   ### How was this patch tested?
   <!--
   If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
   If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
   If tests were not added, please describe why they were not added and/or why it was difficult to add.
   -->
   
   This patch includes two simple tests loading MetaStoreClient via `HiveMetaStoreClientFactory`.
   
   ---
   
   Note for those familiar with the earlier patch:
   
   - Regarding the design decision about MetaStoreClientFactory:
   This patch does not support a customizable MSC factory as the original patch did; instead, it supports a customizable MSC. In other words, this patch introduces `metastore.client.class`, not `metastore.client.factory.class`.
   I made this change not because I preferred this approach, but because I followed the majority opinion in the discussion [here]( https://github.com/apache/hive/pull/4444#pullrequestreview-1492711587). Personally, I think both approaches are valid and have their merits, and I do not have a strong opinion among them. I would appreciate it if you could share your thoughts on this decision.
   
   - Regarding RetryingMetaStoreClient and other classes that instantiate MetaStoreClient ([link](https://github.com/apache/hive/pull/4444#pullrequestreview-1687155221)):
   After resolving HIVE-27473, Hive now creates ThriftHiveMetaStoreClient only in `Hive.java` and `HiveMetaStoreClient`. Therefore, I believe it is sufficient to replace these two call sites with the new `HiveMetaStoreClientFactory`.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3077115461

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5955) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1525 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5955&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5955&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5955&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5955&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5955&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5955)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2209349397


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   > From my point of view, I would suggest the pattern(the database or table) or the catalog routed way to choose which the client will use to obtain the metadata.
   
   I believe this is somewhat similar to the design proposed in HIVE-28879 (Federated Catalog).
   From my perspective, I see HIVE-12679 as a milestone for both HIVE-28658 (Iceberg REST Catalog, #5628) and HIVE-28879. So, I'm fine with moving directly to HIVE-28879 or pursuing any other approach, as long as we aim to support third-party data catalogs. However, since this ticket predates my contributions and has many watchers, I want to ask for and respect others' opinions as well.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "moomindani (via GitHub)" <gi...@apache.org>.
moomindani commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2206166468


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   Thanks, LGTM, I have no concerns around this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2227080066


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   how about a new property to configure those mappings? e.g, `metastore.client.meta.class.mappings`=`catalog:client_classname,catalog.db:client_classname,catalog.db.table:client_classname`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2227823924


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   I don't think we need multiple client impl for the same catalog (aka Snowflake, Glue, HMS, Rest, etc) 
   Instead, I’d suggest using a simple comma-separated list. Later, that would be persisted to the backend database during the Catalog registration (HIVE-26227)
   ````
   METASTORE_CLIENT_IMPL("metastore.client.impl", catalog:client_classname)
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224587757


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -82,19 +87,35 @@ private static IMetaStoreClient createUnderlyingClient(Configuration conf, HiveM
   // methods for test
 
   public boolean createType(Type type) throws TException {
-    return thriftClient.createType(type);
+    if (thriftClient != null) {

Review Comment:
   how about using generic method?
   ````
   @FunctionalInterface
   interface ThriftCallable<T> {
     T call() throws TException;
   }
   
   private <T> T doCall(ThriftCallable<T> callable) throws TException {
     if (thriftClient != null) {
       return callable.call();
     } else {
       throw new UnsupportedOperationException();
     }
   }
   
   public boolean createType(Type type) throws TException {
     return doCall(() -> thriftClient.createType(type));
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3060832069

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5955) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [20 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5955&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5955&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5955&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5955&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5955&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5955)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230230922


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -61,19 +61,24 @@ public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader) th
 
   public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader, Boolean allowEmbedded)
     throws MetaException {
-    this(conf, hookLoader, new ThriftHiveMetaStoreClient(conf, allowEmbedded));
+    this(conf, hookLoader, HiveMetaStoreClientBuilder.createClient(conf, allowEmbedded));

Review Comment:
   fixed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224615580


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   IMO, [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879) will likely require significantly more effort, so we can consider this PR as a intermediate step forward.
   however, maybe we can introduce a static [catalog pattern -> client] mapping here, WDYT?



##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   IMO, [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879) will likely require significantly more effort, so we can consider this PR as a intermediate step forward.
   however, maybe we can introduce a static [catalog -> client] mapping here, WDYT?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2227831813


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   Maybe we should address this in [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879), since we also need to provide connection details per catalog



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224599472


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   @ngsg maybe `METASTORE_CLIENT_IMPL`, similar to `RAW_STORE_IMPL`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2227080066


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   how about a new property to configure those mappings? e.g, `metastore.client.meta.class.mapping`=`catalog:client_classname,catalog.db:client_classname,catalog.db.table:client_classname`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224508598


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java:
##########
@@ -18,65 +18,89 @@
 
 package org.apache.hadoop.hive.metastore.client.builder;
 
+import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hive.metastore.HiveMetaHookLoader;
-import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
 import org.apache.hadoop.hive.metastore.IMetaStoreClient;
 import org.apache.hadoop.hive.metastore.RetryingMetaStoreClient;
 import org.apache.hadoop.hive.metastore.api.MetaException;
 import org.apache.hadoop.hive.metastore.client.HookEnabledMetaStoreClient;
 import org.apache.hadoop.hive.metastore.client.SynchronizedMetaStoreClient;
 import org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.utils.JavaUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.Function;
 
 public class HiveMetaStoreClientBuilder {
+  private static final Logger LOG = LoggerFactory.getLogger(HiveMetaStoreClientBuilder.class);
 
-    private final Configuration conf;
-    private IMetaStoreClient client;
+  private final Configuration conf;
+  private IMetaStoreClient client;
 
-    public HiveMetaStoreClientBuilder(Configuration conf) {
-        this.conf = Objects.requireNonNull(conf);
-    }
+  public HiveMetaStoreClientBuilder(Configuration conf) {
+    this.conf = Objects.requireNonNull(conf);
+  }
 
-    public HiveMetaStoreClientBuilder newClient() throws MetaException {
-        this.client = new HiveMetaStoreClient(conf);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder newClient(boolean allowEmbedded) throws MetaException {
+    this.client = createClient(conf, allowEmbedded);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder newThriftClient(boolean allowEmbedded) throws MetaException {
-        this.client = ThriftHiveMetaStoreClient.newClient(conf, allowEmbedded);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder client(IMetaStoreClient client) {
+    this.client = client;
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder client(IMetaStoreClient client) {
-        this.client = client;
-        return this;
-    }
+  public HiveMetaStoreClientBuilder enhanceWith(Function<IMetaStoreClient, IMetaStoreClient> wrapperFunction) {
+    client = wrapperFunction.apply(client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder enhanceWith(Function<IMetaStoreClient, IMetaStoreClient> wrapperFunction) {
-        client = wrapperFunction.apply(client);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder withHooks(HiveMetaHookLoader hookLoader) {
+    this.client = HookEnabledMetaStoreClient.newClient(conf, hookLoader, client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder withHooks(HiveMetaHookLoader hookLoader) {
-        this.client = HookEnabledMetaStoreClient.newClient(conf, hookLoader, client);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder withRetry(Map<String, Long> metaCallTimeMap) throws MetaException {
+    client = RetryingMetaStoreClient.getProxy(conf, metaCallTimeMap, client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder withRetry(Map<String, Long> metaCallTimeMap) throws MetaException {
-        client = RetryingMetaStoreClient.getProxy(conf, metaCallTimeMap, client);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder threadSafe() {
+    this.client = SynchronizedMetaStoreClient.newClient(conf, client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder threadSafe() {
-        this.client = SynchronizedMetaStoreClient.newClient(conf, client);
-        return this;
-    }
+  public IMetaStoreClient build() {
+    return Objects.requireNonNull(client);
+  }
 
-    public IMetaStoreClient build() {
-        return Objects.requireNonNull(client);
+  public static IMetaStoreClient createClient(Configuration conf, boolean allowEmbedded) throws MetaException {

Review Comment:
   should be private



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224615580


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   IMO, [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879) will likely require significantly more effort, so we can consider this PR as a intermediate step forward.
   however, maybe we can introduce a db -> client mapping here, WDYT?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224531680


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -61,19 +61,24 @@ public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader) th
 
   public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader, Boolean allowEmbedded)
     throws MetaException {
-    this(conf, hookLoader, new ThriftHiveMetaStoreClient(conf, allowEmbedded));
+    this(conf, hookLoader, HiveMetaStoreClientBuilder.createClient(conf, allowEmbedded));
   }
 
   private HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader,
-      ThriftHiveMetaStoreClient thriftClient) {
-    super(createUnderlyingClient(conf, hookLoader, thriftClient), conf);
-    this.thriftClient = thriftClient;
+      IMetaStoreClient baseMetaStoreClient) {
+    super(createUnderlyingClient(conf, hookLoader, baseMetaStoreClient), conf);
+
+    if (baseMetaStoreClient instanceof ThriftHiveMetaStoreClient) {

Review Comment:
   ````
   if (baseMetaStoreClient instanceof ThriftHiveMetaStoreClient metaStoreClient) {
     this.thriftClient = metaStoreClient;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224515899


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -61,19 +61,24 @@ public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader) th
 
   public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader, Boolean allowEmbedded)
     throws MetaException {
-    this(conf, hookLoader, new ThriftHiveMetaStoreClient(conf, allowEmbedded));
+    this(conf, hookLoader, HiveMetaStoreClientBuilder.createClient(conf, allowEmbedded));

Review Comment:
   `new HiveMetaStoreClientBuilder(conf).newClient(allowEmbedded).build()`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230236924


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   > @ngsg maybe `METASTORE_CLIENT_IMPL`, similar to `RAW_STORE_IMPL`?
   
   I changed METASTORE_CLINT_CLASS to METASTORE_CLIENT_IMPL and metastore.client.class to metastore.client.impl.
   
   ---
   
   > how about a new property to configure those mappings? e.g, `metastore.client.meta.class.mappings`=`catalog:client_classname,catalog.db(or db_pattern):client_classname,catalog.db.table:client_classname`
   
   Regarding the mapping, if there were no ThriftHiveMetaStoreClient-related logic in HiveMetaStoreClient, I think I could implement a mapping from a catalog to the actual client. However, due to that logic, I don't currently have a clear design to support it. Let me think more about a better design and share my thoughts once I’ve summarized them.
   
   ---
   
   Another thought I had is that if we eventually implement HIVE-28879, we may not need this configuration key to specify either a single class or a mapping, since the relevant information would be stored in the MetaStore backend database. Given that we likely have enough time before the next release, I'm unsure about the feasibility of introducing a temporary configuration key that won't appear in any release. What do you think about keeping this patch as a fallback option in case HIVE-28879 isn't ready in time for the next release, and proceeding directly with HIVE-28879 for now?
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2205963606


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/client/HiveMetaStoreClientFactory.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore.client;
+
+import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.utils.JavaUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A factory class creating a MetaStoreClient specified in a given configuration.
+ */
+public class HiveMetaStoreClientFactory {
+  private static final Logger LOG = LoggerFactory.getLogger(HiveMetaStoreClientFactory.class);
+
+  public static IMetaStoreClient newClient(Configuration conf, boolean allowEmbedded) throws MetaException {
+    String mscClassName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.METASTORE_CLIENT_CLASS);

Review Comment:
   Sure, I'll modify it to use `getClass` once HIVE-20189 is merged.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2208998466


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   I have a concern here: if we specify the client via the configuration, it means each time we are allowed to query one type of metadata only. For example, we set the client to Glue client for requesting the tables stored in Glue, if we need to request the tables stored in Hive later on in the same session, we might need to reset the client to Hive, and just think loud, if the SQL contains the Hive and Glue table?
   
   From my point of view, I would suggest the pattern(the database or table) or the catalog routed way to choose which the client will use to obtain the metadata.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230236924


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   > @ngsg maybe `METASTORE_CLIENT_IMPL`, similar to `RAW_STORE_IMPL`?
   
   I changed `METASTORE_CLINT_CLASS` to `METASTORE_CLIENT_IMPL` and `metastore.client.class` to `metastore.client.impl`.
   
   ---
   
   > how about a new property to configure those mappings? e.g, `metastore.client.meta.class.mappings`=`catalog:client_classname,catalog.db(or db_pattern):client_classname,catalog.db.table:client_classname`
   
   Regarding the mapping, if there were no ThriftHiveMetaStoreClient-related logic in HiveMetaStoreClient, I think I could implement a mapping from a catalog to the actual client. However, due to that logic, I don't currently have a clear design to support it. Let me think more about a better design and share my thoughts once I’ve summarized them.
   
   ---
   
   Another thought I had is that if we eventually implement HIVE-28879, we may not need this configuration key to specify either a single class or a mapping, since the relevant information would be stored in the MetaStore backend database. Given that we likely have enough time before the next release, I'm unsure about the feasibility of introducing a temporary configuration key that won't appear in any release. What do you think about keeping this patch as a fallback option in case HIVE-28879 isn't ready in time for the next release, and proceeding directly with HIVE-28879 for now?
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2227080066


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   how about a new property to configure those mappings? e.g, `metastore.client.meta.class.mappings`=`catalog:client_classname,catalog.db(or db_pattern):client_classname,catalog.db.table:client_classname`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg merged PR #5955:
URL: https://github.com/apache/hive/pull/5955


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2208998466


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   I have a concern here: if we specify the client via the configuration, it means each time we are allowed to query one type of metadata only. For example, we set the client to Glue for the tables stored in Glue, if we need the Hive tables later on in the same session, we should reset the client to Hive, and just think loud, if the SQL contains the Hive and Glue table?
   
   From my point of view, I would suggest the pattern(the database or table) or the catalog routed way to choose which the client will use to obtain the metadata.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3073230084

   [HIVE-20189](https://issues.apache.org/jira/browse/HIVE-20189) is merged, @ngsg could you please rebase


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224531680


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -61,19 +61,24 @@ public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader) th
 
   public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader, Boolean allowEmbedded)
     throws MetaException {
-    this(conf, hookLoader, new ThriftHiveMetaStoreClient(conf, allowEmbedded));
+    this(conf, hookLoader, HiveMetaStoreClientBuilder.createClient(conf, allowEmbedded));
   }
 
   private HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader,
-      ThriftHiveMetaStoreClient thriftClient) {
-    super(createUnderlyingClient(conf, hookLoader, thriftClient), conf);
-    this.thriftClient = thriftClient;
+      IMetaStoreClient baseMetaStoreClient) {
+    super(createUnderlyingClient(conf, hookLoader, baseMetaStoreClient), conf);
+
+    if (baseMetaStoreClient instanceof ThriftHiveMetaStoreClient) {

Review Comment:
   ````
   if (baseMetaStoreClient instanceof ThriftHiveMetaStoreClient metaStoreClient) {
     this.thriftClient = metaStoreClient;
   }
   ````
   and drop the final



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3116532061

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5955) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [23 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5955&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5955&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5955&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5955&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5955&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5955)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230231960


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -82,19 +87,35 @@ private static IMetaStoreClient createUnderlyingClient(Configuration conf, HiveM
   // methods for test
 
   public boolean createType(Type type) throws TException {
-    return thriftClient.createType(type);
+    if (thriftClient != null) {

Review Comment:
   Fixed to use generic method.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2203163029


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/client/HiveMetaStoreClientFactory.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore.client;
+
+import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.utils.JavaUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A factory class creating a MetaStoreClient specified in a given configuration.
+ */
+public class HiveMetaStoreClientFactory {
+  private static final Logger LOG = LoggerFactory.getLogger(HiveMetaStoreClientFactory.class);
+
+  public static IMetaStoreClient newClient(Configuration conf, boolean allowEmbedded) throws MetaException {
+    String mscClassName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.METASTORE_CLIENT_CLASS);

Review Comment:
   Can we use MetastoreConf.getClass?



##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   @moomindani Please feel free to give us a suggestion if you have any thoughts.
   
   > Regarding the design decision about MetaStoreClientFactory:
   This patch does not support a customizable MSC factory as the original patch did; instead, it supports a customizable MSC. In other words, this patch introduces metastore.client.class, not metastore.client.factory.class.
   I made this change not because I preferred this approach, but because I followed the majority opinion in the discussion https://github.com/apache/hive/pull/4444#pullrequestreview-1492711587. Personally, I think both approaches are valid and have their merits, and I do not have a strong opinion among them. I would appreciate it if you could share your thoughts on this decision.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3060667652

   Hi @ngsg, let's get https://github.com/apache/hive/pull/5924 in first. Please review it when time allows. 
   Thank you!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230755791


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   > Another thought I had is that if we eventually implement [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879), we may not need this configuration key to specify either a single class or a mapping, since the relevant information would be stored in the MetaStore backend database.
   
   Totally agree + the proposed mapping might require connection details as well. I think we should go ahead and merge this PR, and start thinking on a more generic solution like [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3138272885

   @ayushtkn,  thanks for the reminder. I'll merge it shortly.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3138358577

   Thanks all for the review!
   
   ---
   
   I have read the [guide](https://hive.apache.org/docs/latest/howtocommit_27362108/) and tried to follow the documented process for merging. Since this is my first time merging, I would appreciate it if someone could check whether I've made any mistakes I might have overlooked.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230230476


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java:
##########
@@ -18,65 +18,89 @@
 
 package org.apache.hadoop.hive.metastore.client.builder;
 
+import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hive.metastore.HiveMetaHookLoader;
-import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
 import org.apache.hadoop.hive.metastore.IMetaStoreClient;
 import org.apache.hadoop.hive.metastore.RetryingMetaStoreClient;
 import org.apache.hadoop.hive.metastore.api.MetaException;
 import org.apache.hadoop.hive.metastore.client.HookEnabledMetaStoreClient;
 import org.apache.hadoop.hive.metastore.client.SynchronizedMetaStoreClient;
 import org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.utils.JavaUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.Function;
 
 public class HiveMetaStoreClientBuilder {
+  private static final Logger LOG = LoggerFactory.getLogger(HiveMetaStoreClientBuilder.class);
 
-    private final Configuration conf;
-    private IMetaStoreClient client;
+  private final Configuration conf;
+  private IMetaStoreClient client;
 
-    public HiveMetaStoreClientBuilder(Configuration conf) {
-        this.conf = Objects.requireNonNull(conf);
-    }
+  public HiveMetaStoreClientBuilder(Configuration conf) {
+    this.conf = Objects.requireNonNull(conf);
+  }
 
-    public HiveMetaStoreClientBuilder newClient() throws MetaException {
-        this.client = new HiveMetaStoreClient(conf);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder newClient(boolean allowEmbedded) throws MetaException {
+    this.client = createClient(conf, allowEmbedded);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder newThriftClient(boolean allowEmbedded) throws MetaException {
-        this.client = ThriftHiveMetaStoreClient.newClient(conf, allowEmbedded);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder client(IMetaStoreClient client) {
+    this.client = client;
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder client(IMetaStoreClient client) {
-        this.client = client;
-        return this;
-    }
+  public HiveMetaStoreClientBuilder enhanceWith(Function<IMetaStoreClient, IMetaStoreClient> wrapperFunction) {
+    client = wrapperFunction.apply(client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder enhanceWith(Function<IMetaStoreClient, IMetaStoreClient> wrapperFunction) {
-        client = wrapperFunction.apply(client);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder withHooks(HiveMetaHookLoader hookLoader) {
+    this.client = HookEnabledMetaStoreClient.newClient(conf, hookLoader, client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder withHooks(HiveMetaHookLoader hookLoader) {
-        this.client = HookEnabledMetaStoreClient.newClient(conf, hookLoader, client);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder withRetry(Map<String, Long> metaCallTimeMap) throws MetaException {
+    client = RetryingMetaStoreClient.getProxy(conf, metaCallTimeMap, client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder withRetry(Map<String, Long> metaCallTimeMap) throws MetaException {
-        client = RetryingMetaStoreClient.getProxy(conf, metaCallTimeMap, client);
-        return this;
-    }
+  public HiveMetaStoreClientBuilder threadSafe() {
+    this.client = SynchronizedMetaStoreClient.newClient(conf, client);
+    return this;
+  }
 
-    public HiveMetaStoreClientBuilder threadSafe() {
-        this.client = SynchronizedMetaStoreClient.newClient(conf, client);
-        return this;
-    }
+  public IMetaStoreClient build() {
+    return Objects.requireNonNull(client);
+  }
 
-    public IMetaStoreClient build() {
-        return Objects.requireNonNull(client);
+  public static IMetaStoreClient createClient(Configuration conf, boolean allowEmbedded) throws MetaException {

Review Comment:
   fixed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ngsg (via GitHub)" <gi...@apache.org>.
ngsg commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2230231398


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -61,19 +61,24 @@ public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader) th
 
   public HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader, Boolean allowEmbedded)
     throws MetaException {
-    this(conf, hookLoader, new ThriftHiveMetaStoreClient(conf, allowEmbedded));
+    this(conf, hookLoader, HiveMetaStoreClientBuilder.createClient(conf, allowEmbedded));
   }
 
   private HiveMetaStoreClient(Configuration conf, HiveMetaHookLoader hookLoader,
-      ThriftHiveMetaStoreClient thriftClient) {
-    super(createUnderlyingClient(conf, hookLoader, thriftClient), conf);
-    this.thriftClient = thriftClient;
+      IMetaStoreClient baseMetaStoreClient) {
+    super(createUnderlyingClient(conf, hookLoader, baseMetaStoreClient), conf);
+
+    if (baseMetaStoreClient instanceof ThriftHiveMetaStoreClient) {

Review Comment:
   Fixed as suggested. I wasn't aware of that syntax before. Thank you for the helpful suggestion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3138776606

   > I have read the [guide](https://hive.apache.org/docs/latest/howtocommit_27362108/) and tried to follow the documented process for merging. Since this is my first time merging, I would appreciate it if someone could check whether I've made any mistakes I might have overlooked.
   
   It is all good


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on PR #5955:
URL: https://github.com/apache/hive/pull/5955#issuecomment-3137818606

   This seems good to go? if yes, @ngsg mind hitting the merge button?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224587757


##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java:
##########
@@ -82,19 +87,35 @@ private static IMetaStoreClient createUnderlyingClient(Configuration conf, HiveM
   // methods for test
 
   public boolean createType(Type type) throws TException {
-    return thriftClient.createType(type);
+    if (thriftClient != null) {

Review Comment:
   how about using generic method?
   ````
     @FunctionalInterface
     interface ThriftCallable<T> {
       T call() throws TException;
     }
   
     private <T> T doCall(ThriftCallable<T> callable) throws TException {
       if (thriftClient != null) {
         return callable.call();
       } else {
         throw new UnsupportedOperationException();
       }
     }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224615580


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   IMO, [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879) will likely require significantly more effort, so we can consider this PR as a intermediate step forward.
   however, maybe we can introduce a static [db pattern -> client] mapping here, WDYT?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224615580


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   IMO, [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879) will likely require significantly more effort, so we can consider this PR as a intermediate step forward.
   however, maybe we can introduce a static [DbCatalog -> Client] mapping here, WDYT?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5955:
URL: https://github.com/apache/hive/pull/5955#discussion_r2224615580


##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1728,6 +1728,10 @@ public enum ConfVars {
                     " and password. Any other value is ignored right now but may be used later."
                 + "If JWT- Supported only in HTTP transport mode. If set, HMS Client will pick the value of JWT from "
                 + "environment variable HMS_JWT and set it in Authorization header in http request"),
+    METASTORE_CLIENT_CLASS("metastore.client.class",
+        "hive.metastore.client.class",
+        "org.apache.hadoop.hive.metastore.client.ThriftHiveMetaStoreClient",
+        "The name of MetaStoreClient class that implements the IMetaStoreClient interface."),

Review Comment:
   IMO, [HIVE-28879](https://issues.apache.org/jira/browse/HIVE-28879) will likely require significantly more effort, so we can consider this PR as a intermediate step forward.
   however, maybe we can introduce a [db pattern -> client] mapping here, WDYT?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org