saihemanth-cloudera commented on code in PR #6585:
URL: https://github.com/apache/hive/pull/6585#discussion_r3546602197
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/RawStoreBundle.java:
##########
@@ -43,4 +43,8 @@ public RawStore getBaseStore() {
public PersistenceManager getPersistentManager() {
return pm;
}
+
+ protected <T> T siblingStore(Class<T> iface) {
Review Comment:
Just to be safe: Are these sibling stores guaranteed to stay bounded within
the lifetime of the parent RawStoreBundle? We need to ensure that a reference
to a sibling store is never leaked outside the current transaction thread,
otherwise, we might accidentally introduce a different type of stale-data or
memory leak.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java:
##########
@@ -412,27 +410,23 @@ public boolean openTransaction() {
return result;
}
- @SuppressWarnings("unchecked")
+ @SuppressWarnings("unchecked, rawtypes")
@Override
public <T> T unwrap(Class<T> iface) {
+ if (pm == null) {
+ throw new IllegalStateException("ObjectStore hasn't been initialized
yet");
+ }
MetaDescriptor descriptor = iface.getAnnotation(MetaDescriptor.class);
if (descriptor == null) {
throw new IllegalArgumentException("Unable to unwrap the store as " +
iface);
}
- String implClassName = conf.get("metastore." + descriptor.alias() +
".store.impl", "");
- T simpl;
- T impl = (T) cachedImpls.get(iface);
Review Comment:
By removing the implementation cache and creating a fresh store instance on
every single unwrap() call, we completely solve the state pollution bug.
However, let's double-check how frequently unwrap() is invoked across the
codebase. If unwrap() is called inside hot loops (for example, iterating over
thousands of partitions during a large SELECT query), generating new instances
repeatedly could significantly increase garbage collection (GC) pressure.
Can we verify that unwrap() is typically called only once or twice per
metadata operation lifecycle rather than per-item in a loop?
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/metastore/TestObjectStoreUnwrap.java:
##########
@@ -0,0 +1,327 @@
+/*
+ * 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.metastore;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.metastore.Deadline;
+import org.apache.hadoop.hive.metastore.HMSHandler;
+import org.apache.hadoop.hive.metastore.MetaStoreTestUtils;
+import org.apache.hadoop.hive.metastore.ObjectStore;
+import org.apache.hadoop.hive.metastore.Warehouse;
+import org.apache.hadoop.hive.metastore.annotation.MetastoreUnitTest;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.client.builder.DatabaseBuilder;
+import org.apache.hadoop.hive.metastore.client.builder.GetPartitionsArgs;
+import org.apache.hadoop.hive.metastore.client.builder.PartitionBuilder;
+import org.apache.hadoop.hive.metastore.client.builder.TableBuilder;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars;
+import org.apache.hadoop.hive.metastore.metastore.iface.TableStore;
+import org.apache.hadoop.hive.metastore.utils.DirectSqlConfigurator;
+import org.apache.hadoop.hive.metastore.utils.MetaStoreServerUtils;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.lang.reflect.Proxy;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.UUID;
+
+import static org.apache.hadoop.hive.metastore.Warehouse.DEFAULT_CATALOG_NAME;
+
+@Category(MetastoreUnitTest.class)
+public class TestObjectStoreUnwrap {
Review Comment:
Can we add one more test method where multiple threads concurrently fetch
unwrapped stores and execute queries, ensuring that Thread A’s transaction
cleanup never prematurely closes or leaks Thread B’s queries?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
