HIVE-20189: Separate metastore client code into its own module #5924

deniskuzZ · 2025-07-03T09:19:47Z

What changes were proposed in this pull request?

move client classes into it's own module

Why are the changes needed?

improve the structure of standalone-metastore classes

Does this PR introduce any user-facing change?

No

How was this patch tested?

jenkins

dengzhhu653 · 2025-07-03T11:14:07Z

standalone-metastore/metastore-client/pom.xml

+  <dependencies>
+    <dependency>
+      <groupId>org.apache.hive</groupId>
+      <artifactId>hive-standalone-metastore-common</artifactId>


The new module depends on the metastore-common, which doesn't reduce the size or dependencies from metastore-common, not sure if it's ok, for the user what will benefit from the new module?

It's expected for a client module to depend on a common module. The client module delivers core client functionality and can be enhanced later with features like caching.
Offering a 'client' JAR aligns with common conventions and is expected to be user-friendly.
ATM we have 2 distinct cache wrappers in different Hive modules, just because there was no structure:

org.apache.hadoop.hive.metastore.HiveClientCache

org.apache.iceberg.hive.CachedClientPool

another point I don't understand, why ql and beeline modules depend on metastore-server? I think we should drop the dependency on server and move the classes into the metastore common or client.

I basically agree that we can potentially improve the structure in the future. We will have obvious guidelines about how to organize files.

metastore-client: Client-specific files, which Server doesn't need

metastore-server: Server-specific files, which Client doesn't need

metastore-common: Common modules

another point I don't understand, why ql and beeline modules depend on metastore-server? I think we should drop the dependency on server and move the classes into the metastore common or client.

I guess ql requires metastore-server to use an embedded HMS.

isn't embedded HMS only used in tests?

No, it isn't. It can be used when metastore.thrift.uris is empty. For example, our Hive docker image(not HMS docker image) can set up a HiveServer2 without HMS. It probably uses the embedded mode that runs HMS-equivalent threads in HS2.

hive/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java

Lines 2654 to 2663 in d7b3a5b

/**

* Check if metastore is being used in embedded mode.

* This utility function exists so that the logic for determining the mode is same

* in HiveConf and HiveMetaStoreClient

* @param msUri - metastore server uri

* @return true if the metastore is embedded

*/

public static boolean isEmbeddedMetaStore(String msUri) {

return (msUri == null) || msUri.trim().isEmpty();

}

Alternatively, we can say it is for testing purposes and separate the classes from hive-exec.

deniskuzZ · 2025-07-11T05:54:52Z

I've kept the package unchanged for IMetaStoreClient, HiveMetaStoreClient, RetryingMetaStoreClient. Ideally, they should be moved under the client package.
@dengzhhu653, @ngsg, @okumin, WDYT should we refactor here (not a small PR already) or in a follow-up?

ngsg

The patch looks reasonable to me. I quickly checked whether there are any remaining candidates that could be moved to standalone-metastore-client, and it seems that all client-only-related classes have been gathered into the client package. I'm also in favor of the new HiveMetaStoreClientBuilder class.

I left a few minor comments where I had questions. Also, it seems that the CI failed due to the unification of HiveClientCache. Could you update the TestPassProperties#testSequenceTableWriteReadMR accordingly?

Regarding IMetaStoreClient, HiveMetaStoreClient, and RetryingMetaStoreClient, I'm leaning towards refactoring them in a follow-up PR, as the large refactoring might obscure the existing changes.

ngsg · 2025-07-11T07:51:36Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMetaStoreClientWithLocalCache.java

@@ -41,7 +41,7 @@
 import org.apache.hadoop.hive.metastore.api.PartitionsByExprRequest;
 import org.apache.hadoop.hive.metastore.api.PartitionsStatsRequest;
 import org.apache.hadoop.hive.metastore.api.Table;
-import org.apache.hadoop.hive.metastore.client.HiveMetaStoreClientUtils;
+import org.apache.hadoop.hive.metastore.client.utils.HiveMetaStoreClientUtils;


I think the order of import might be swapped as follows:

import org.apache.hadoop.hive.metastore.client.MetaStoreClientWrapper; import org.apache.hadoop.hive.metastore.client.utils.HiveMetaStoreClientUtils;

ngsg · 2025-07-11T07:52:43Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java

@@ -92,7 +92,7 @@
 import org.apache.hadoop.hive.metastore.api.TableValidWriteIds;
 import org.apache.hadoop.hive.metastore.api.UniqueConstraintsRequest;
 import org.apache.hadoop.hive.metastore.api.UniqueConstraintsResponse;
-import org.apache.hadoop.hive.metastore.client.HiveMetaStoreClientUtils;
+import org.apache.hadoop.hive.metastore.client.utils.HiveMetaStoreClientUtils;


The same import order issue here.

ngsg · 2025-07-11T07:59:16Z

...metastore-client/src/main/java/org/apache/hadoop/hive/metastore/RetryingMetaStoreClient.java

@@ -65,15 +67,21 @@ public class RetryingMetaStoreClient implements InvocationHandler {
  private final UserGroupInformation ugi;
  private final int retryLimit;
  private final long retryDelaySeconds;
-  private final ConcurrentHashMap<String, Long> metaCallTimeMap;
+  private final Map<String, Long> metaCallTimeMap;


What do you think about removing all uses of ConcurrentHashMap from this file? It seems that only one method uses ConcurrentHashMap, and the actual value passed to that method is always null.

ngsg · 2025-07-11T08:11:34Z

...metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/TableIterable.java

It seems that the tab size is set to 4 inside Iterator { }. Could we restore it to 2?

ngsg · 2025-07-11T08:23:48Z

...rc/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java

+import org.apache.hadoop.hive.metastore.HiveMetaHookLoader;
+import org.apache.hadoop.hive.metastore.IMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+


Could we remove the empty line and sort the import list?

sonarqubecloud · 2025-07-12T03:12:06Z

Quality Gate passed

Issues
1515 New issues
0 Accepted issues

Measures
1 Security Hotspot
0.0% Coverage on New Code
0.9% Duplication on New Code

See analysis details on SonarQube Cloud

okumin · 2025-07-13T06:06:12Z

...rc/main/java/org/apache/hadoop/hive/metastore/client/builder/HiveMetaStoreClientBuilder.java

+
+    public HiveMetaStoreClientBuilder(Configuration conf) {
+        Preconditions.checkNotNull(conf);
+        this.conf = conf;


You can simplify it with one-line like this.conf = Preconditions.checkNotNull(conf). I prefer Objects.requireNonNull for new code, though.

okumin · 2025-07-13T06:17:00Z

metastore/src/java/org/apache/hadoop/hive/metastore/HiveClientCache.java

@@ -107,7 +107,7 @@ public HiveClientCache(final int timeout) {
  /**
   * @param timeout the length of time in seconds after a client is created that it should be automatically removed
   */
-  private HiveClientCache(final int timeout, final int initialCapacity, final int maxCapacity, final boolean enableStats) {
+  protected HiveClientCache(final int timeout, final int initialCapacity, final int maxCapacity, final boolean enableStats) {


Is it simpler to make this public and directly instantiate HiveClientCache in HCatUtil?

deniskuzZ requested a review from dengzhhu653 July 3, 2025 09:19

asf-ci-hive added the tests pending label Jul 3, 2025

deniskuzZ mentioned this pull request Jul 3, 2025

HIVE-29062: Add standalone module for packaging the Metastore #5923

Merged

asf-ci-hive added tests failed and removed tests pending labels Jul 3, 2025

dengzhhu653 reviewed Jul 3, 2025

View reviewed changes

deniskuzZ force-pushed the HIVE-20189 branch 2 times, most recently from 5461f07 to 24ca325 Compare July 3, 2025 13:10

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jul 3, 2025

deniskuzZ force-pushed the HIVE-20189 branch 3 times, most recently from 959dfa7 to bc40344 Compare July 3, 2025 18:26

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jul 3, 2025

deniskuzZ force-pushed the HIVE-20189 branch from 89b0d74 to 8374043 Compare July 5, 2025 18:20

asf-ci-hive added tests pending and removed tests failed labels Jul 5, 2025

deniskuzZ force-pushed the HIVE-20189 branch from 7761afe to 5613b90 Compare July 10, 2025 21:36

deniskuzZ added 2 commits July 11, 2025 00:51

HIVE-20189: Separate metastore client code into its own module

8ba3561

updated the compiler and javadoc plugin versions

83bd985

deniskuzZ force-pushed the HIVE-20189 branch from 5613b90 to 83bd985 Compare July 10, 2025 21:51

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending tests unstable labels Jul 10, 2025

deniskuzZ mentioned this pull request Jul 11, 2025

HIVE-12679: Allow users to be able to specify an implementation of IMetaStoreClient via HiveConf #5955

Open

ngsg reviewed Jul 11, 2025

View reviewed changes

deniskuzZ force-pushed the HIVE-20189 branch 3 times, most recently from 9593477 to 9598be2 Compare July 11, 2025 12:00

asf-ci-hive added tests failed tests pending and removed tests pending tests failed labels Jul 11, 2025

review comments iter#1

f3ea566

deniskuzZ force-pushed the HIVE-20189 branch from 9598be2 to f3ea566 Compare July 11, 2025 19:57

asf-ci-hive added tests pending and removed tests failed labels Jul 11, 2025

asf-ci-hive added tests passed and removed tests pending labels Jul 12, 2025

okumin reviewed Jul 13, 2025

View reviewed changes

	/**
	* Check if metastore is being used in embedded mode.
	* This utility function exists so that the logic for determining the mode is same
	* in HiveConf and HiveMetaStoreClient
	* @param msUri - metastore server uri
	* @return true if the metastore is embedded
	*/
	public static boolean isEmbeddedMetaStore(String msUri) {
	return (msUri == null) \|\| msUri.trim().isEmpty();
	}

HIVE-20189: Separate metastore client code into its own module #5924

Are you sure you want to change the base?

HIVE-20189: Separate metastore client code into its own module #5924

Conversation

deniskuzZ commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniskuzZ Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniskuzZ Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniskuzZ commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngsg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 12, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deniskuzZ commented Jul 3, 2025 •

edited

Loading

deniskuzZ Jul 3, 2025 •

edited

Loading

deniskuzZ Jul 3, 2025 •

edited

Loading

deniskuzZ commented Jul 11, 2025 •

edited

Loading

ngsg left a comment •

edited

Loading