From c610d656bcf218f437716fa287de7e5d434c47eb Mon Sep 17 00:00:00 2001 From: zdeng Date: Thu, 15 Jan 2026 17:36:14 +0800 Subject: [PATCH] HIVE-29409: Async request fails to retry on transparent exceptions --- .../client/ThriftHiveMetaStoreClient.java | 2 + standalone-metastore/metastore-server/pom.xml | 1 - .../hadoop/hive/metastore/HMSHandler.java | 10 + .../metastore/handler/CreateTableHandler.java | 12 +- .../handler/DropDatabaseHandler.java | 40 ++-- .../handler/TestRetryOnException.java | 211 ++++++++++++++++++ standalone-metastore/pom.xml | 1 + 7 files changed, 252 insertions(+), 25 deletions(-) create mode 100644 standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/handler/TestRetryOnException.java diff --git a/standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/ThriftHiveMetaStoreClient.java b/standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/ThriftHiveMetaStoreClient.java index 50d9fae59636..563a4a8c99d1 100644 --- a/standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/ThriftHiveMetaStoreClient.java +++ b/standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/ThriftHiveMetaStoreClient.java @@ -1530,6 +1530,7 @@ public void dropDatabase(DropDatabaseRequest req) throws TException { try { while (!resp.isFinished() && !Thread.currentThread().isInterrupted()) { resp = client.drop_database_req(req); + req.setId(resp.getId()); if (resp.getMessage() != null) { LOG.info(resp.getMessage()); } @@ -1706,6 +1707,7 @@ public void dropTable(String catName, String dbname, String name, boolean delete try { while (!resp.isFinished() && !Thread.currentThread().isInterrupted()) { resp = client.drop_table_req(dropTableReq); + dropTableReq.setId(resp.getId()); if (resp.getMessage() != null) { LOG.info(resp.getMessage()); } diff --git a/standalone-metastore/metastore-server/pom.xml b/standalone-metastore/metastore-server/pom.xml index 47783d74f7f4..64926eaba1c9 100644 --- a/standalone-metastore/metastore-server/pom.xml +++ b/standalone-metastore/metastore-server/pom.xml @@ -23,7 +23,6 @@ Hive Metastore Server .. - 0.10.2 diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java index 2a7ce1c94d2a..e8a0a58b643f 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java @@ -1587,6 +1587,10 @@ public AsyncOperationResp drop_database_req(final DropDatabaseRequest req) return status.toAsyncOperationResp(); } catch (Exception e) { ex = e; + // Reset the id of the request in case of RetryingHMSHandler retries + if (req.getId() != null && req.isAsyncDrop() && !req.isCancel()) { + req.setId(null); + } throw handleException(e) .throwIfInstance(NoSuchObjectException.class, InvalidOperationException.class, MetaException.class) .defaultMetaException(); @@ -2429,6 +2433,12 @@ public AsyncOperationResp drop_table_req(DropTableRequest dropReq) return status.toAsyncOperationResp(); } catch (Exception e) { ex = e; + // Here we get an exception, the RetryingHMSHandler might retry the call, + // need to clear the id from the request, so AbstractRequestHandler can + // start a new execution other than reuse the cached failed handler. + if (dropReq.getId() != null && dropReq.isAsyncDrop() && !dropReq.isCancel()) { + dropReq.setId(null); + } throw handleException(e).throwIfInstance(MetaException.class, NoSuchObjectException.class) .convertIfInstance(IOException.class, MetaException.class).defaultMetaException(); } finally { diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/CreateTableHandler.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/CreateTableHandler.java index 35438f3b4667..81da6dc7eb2c 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/CreateTableHandler.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/CreateTableHandler.java @@ -413,27 +413,27 @@ protected void beforeExecute() throws TException, IOException { String catName = tbl.getCatName(); // Check that constraints have catalog name properly set first if (CollectionUtils.isNotEmpty(constraints.getPrimaryKeys()) - && !constraints.getPrimaryKeys().get(0).isSetCatName()) { + && !constraints.getPrimaryKeys().getFirst().isSetCatName()) { constraints.getPrimaryKeys().forEach(constraint -> constraint.setCatName(catName)); } if (CollectionUtils.isNotEmpty(constraints.getForeignKeys()) - && !constraints.getForeignKeys().get(0).isSetCatName()) { + && !constraints.getForeignKeys().getFirst().isSetCatName()) { constraints.getForeignKeys().forEach(constraint -> constraint.setCatName(catName)); } if (CollectionUtils.isNotEmpty(constraints.getUniqueConstraints()) - && !constraints.getUniqueConstraints().get(0).isSetCatName()) { + && !constraints.getUniqueConstraints().getFirst().isSetCatName()) { constraints.getUniqueConstraints().forEach(constraint -> constraint.setCatName(catName)); } if (CollectionUtils.isNotEmpty(constraints.getNotNullConstraints()) - && !constraints.getNotNullConstraints().get(0).isSetCatName()) { + && !constraints.getNotNullConstraints().getFirst().isSetCatName()) { constraints.getNotNullConstraints().forEach(constraint -> constraint.setCatName(catName)); } if (CollectionUtils.isNotEmpty(constraints.getDefaultConstraints()) - && !constraints.getDefaultConstraints().get(0).isSetCatName()) { + && !constraints.getDefaultConstraints().getFirst().isSetCatName()) { constraints.getDefaultConstraints().forEach(constraint -> constraint.setCatName(catName)); } if (CollectionUtils.isNotEmpty(constraints.getCheckConstraints()) - && !constraints.getCheckConstraints().get(0).isSetCatName()) { + && !constraints.getCheckConstraints().getFirst().isSetCatName()) { constraints.getCheckConstraints().forEach(constraint -> constraint.setCatName(catName)); } } diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/DropDatabaseHandler.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/DropDatabaseHandler.java index fb372c8b8de3..3a475040a7f8 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/DropDatabaseHandler.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/DropDatabaseHandler.java @@ -212,24 +212,7 @@ protected void beforeExecute() throws TException, IOException { pkgRequest.setDbName(name); packages = defaultEmptyList(rs.listPackages(pkgRequest)); - if (!request.isCascade()) { - if (!tableNames.isEmpty()) { - throw new InvalidOperationException( - "Database " + db.getName() + " is not empty. One or more tables exist."); - } - if (!functions.isEmpty()) { - throw new InvalidOperationException( - "Database " + db.getName() + " is not empty. One or more functions exist."); - } - if (!procedures.isEmpty()) { - throw new InvalidOperationException( - "Database " + db.getName() + " is not empty. One or more stored procedures exist."); - } - if (!packages.isEmpty()) { - throw new InvalidOperationException( - "Database " + db.getName() + " is not empty. One or more packages exist."); - } - } + validateCascadeDrop(tableNames); Path path = new Path(db.getLocationUri()).getParent(); if (!handler.getWh().isWritable(path)) { throw new MetaException("Database not dropped since its external warehouse location " + path + @@ -271,6 +254,27 @@ private void checkFuncPathToCm() { result.setFunctionCmPaths(funcNeedCmPaths); } + private void validateCascadeDrop(List tableNames) throws InvalidOperationException { + if (!request.isCascade()) { + if (!tableNames.isEmpty()) { + throw new InvalidOperationException( + "Database " + db.getName() + " is not empty. One or more tables exist."); + } + if (!functions.isEmpty()) { + throw new InvalidOperationException( + "Database " + db.getName() + " is not empty. One or more functions exist."); + } + if (!procedures.isEmpty()) { + throw new InvalidOperationException( + "Database " + db.getName() + " is not empty. One or more stored procedures exist."); + } + if (!packages.isEmpty()) { + throw new InvalidOperationException( + "Database " + db.getName() + " is not empty. One or more packages exist."); + } + } + } + private void checkTablePathPermission(RawStore rs, List tableNames) throws MetaException { int tableBatchSize = MetastoreConf.getIntVar(handler.getConf(), MetastoreConf.ConfVars.BATCH_RETRIEVE_MAX); Warehouse wh = handler.getWh(); diff --git a/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/handler/TestRetryOnException.java b/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/handler/TestRetryOnException.java new file mode 100644 index 000000000000..c543dfaec239 --- /dev/null +++ b/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/handler/TestRetryOnException.java @@ -0,0 +1,211 @@ +/* + * 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.handler; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hive.common.TableName; +import org.apache.hadoop.hive.metastore.HiveMetaStoreClient; +import org.apache.hadoop.hive.metastore.TableType; +import org.apache.hadoop.hive.metastore.TransactionalMetaStoreEventListener; +import org.apache.hadoop.hive.metastore.annotation.MetastoreCheckinTest; +import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.metastore.api.NoSuchObjectException; +import org.apache.hadoop.hive.metastore.api.Table; +import org.apache.hadoop.hive.metastore.client.MetaStoreClientTest; +import org.apache.hadoop.hive.metastore.client.builder.DatabaseBuilder; +import org.apache.hadoop.hive.metastore.client.builder.TableBuilder; +import org.apache.hadoop.hive.metastore.conf.MetastoreConf; +import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent; +import org.apache.hadoop.hive.metastore.events.DropTableEvent; +import org.apache.hadoop.hive.metastore.minihms.AbstractMetaStoreService; +import org.apache.thrift.TException; +import org.datanucleus.exceptions.NucleusException; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; + +@RunWith(Parameterized.class) +@Category(MetastoreCheckinTest.class) +public class TestRetryOnException extends MetaStoreClientTest { + protected static final String DB_NAME = "testdroptable"; + protected static final String TABLE_NAME = "test_drop_table"; + private final String testTempDir = + Paths.get(System.getProperty("java.io.tmpdir"), "testDropTable").toString(); + private AbstractMetaStoreService metaStore; + private HiveMetaStoreClient client; + + public TestRetryOnException(String name, AbstractMetaStoreService metaStore) { + this.metaStore = metaStore; + } + + @BeforeClass + public static void startMetaStores() { + Map msConf = new HashMap<>(); + Map extraConf = new HashMap<>(); + extraConf.put("metastore.transactional.event.listeners", AssertExTransactionListener.class.getName()); + startMetaStores(msConf, extraConf); + } + + @Before + public void setUp() throws Exception { + AssertExTransactionListener.resetTest(); + client = metaStore.getClient(); + cleanDB(); + createDB(DB_NAME); + AssertExTransactionListener.startTest(); + } + + @Test + public void testRetryOnException() throws Exception { + String location = metaStore.getExternalWarehouseRoot() + "/" + TABLE_NAME; + new TableBuilder() + .setDbName(DB_NAME) + .setTableName(TABLE_NAME) + .addCol("test_id", "int", "test col id") + .addCol("test_value", "string", "test col value") + .addTableParam("param_key", "param_value") + .setType(TableType.EXTERNAL_TABLE.name()) + .addTableParam("EXTERNAL", "TRUE") + .addStorageDescriptorParam("sd_param_key", "sd_param_value") + .setSerdeName(TABLE_NAME) + .setStoredAsSubDirectories(false) + .addSerdeParam("serde_param_key", "serde_param_value") + .setLocation(location) + .create(client, metaStore.getConf()); + + Table table = client.getTable(DB_NAME, TABLE_NAME); + assertNotNull(table); + String tableName = TableName.getQualified(table.getCatName(), table.getDbName(), table.getTableName()); + + assertEquals(0, AssertExTransactionListener.getCalls(tableName)); + client.dropTable(DB_NAME, TABLE_NAME); + assertEquals(AssertExTransactionListener.maxRetries, AssertExTransactionListener.getCalls(tableName)); + try { + client.getTable(DB_NAME, TABLE_NAME); + fail("Table " + tableName + " should be dropped"); + } catch (NoSuchObjectException nse) { + // expected + } + + assertEquals(0, AssertExTransactionListener.getCalls(DB_NAME)); + client.dropDatabase(DB_NAME); + assertEquals(AssertExTransactionListener.maxRetries, AssertExTransactionListener.getCalls(tableName)); + try { + client.getDatabase(DB_NAME); + fail("Database " + DB_NAME + " should be dropped"); + } catch (NoSuchObjectException nse) { + // expected + } + } + + public static class AssertExTransactionListener extends TransactionalMetaStoreEventListener { + static Map calls = new HashMap<>(); + static int maxRetries = 5; + static boolean startChecking = false; + + public AssertExTransactionListener(Configuration config) { + super(config); + } + + @Override + public void onDropTable(DropTableEvent tableEvent) throws MetaException { + Table table = tableEvent.getTable(); + String tableName = TableName.getQualified(table.getCatName(), table.getDbName(), table.getTableName()); + Integer pre = calls.putIfAbsent(tableName, 1); + if (pre != null) { + calls.put(tableName, pre + 1); + } + // For a drop table call, we should see 5(maxRetries) retries in RetryingHMSHandler + assertException(startChecking && calls.get(tableName) < maxRetries); + } + + @Override + public void onDropDatabase(DropDatabaseEvent dbEvent) throws MetaException { + String dbName = dbEvent.getDatabase().getName(); + Integer pre = calls.putIfAbsent(dbName, 1); + if (pre != null) { + calls.put(dbName, pre + 1); + } + assertException(startChecking && calls.get(dbName) < maxRetries); + } + + private void assertException(boolean throwException) throws MetaException { + if (throwException) { + MetaException exception = new MetaException("An exception for RetryingHMSHandler to retry"); + exception.initCause(new NucleusException("Non-fatal exception")); + throw exception; + } + } + + static void startTest() { + startChecking = true; + calls.clear(); + } + + static void resetTest() { + startChecking = false; + calls.clear(); + } + + static int getCalls(String key) { + return calls.get(key) != null ? calls.get(key) : 0; + } + } + + @After + public void tearDown() throws Exception { + try { + if (client != null) { + try { + client.close(); + } catch (Exception e) { + } + } + } finally { + client = null; + } + + Path path = new Path(testTempDir); + path.getFileSystem(metaStore.getConf()).delete(path, true); + } + + protected void cleanDB() throws Exception{ + client.dropDatabase(DB_NAME, true, true, true); + metaStore.cleanWarehouseDirs(); + } + + protected void createDB(String dbName) throws TException { + new DatabaseBuilder(). + setName(dbName). + create(client, metaStore.getConf()); + } +} diff --git a/standalone-metastore/pom.xml b/standalone-metastore/pom.xml index 8c9f5256f91e..30ca1bf00f4d 100644 --- a/standalone-metastore/pom.xml +++ b/standalone-metastore/pom.xml @@ -101,6 +101,7 @@ 3.25.5 3.11.4 ${env.PROTOC_PATH} + 0.10.2 1.72.0 1.9.0 4.1.127.Final