Skip to content

Commit 7b54334

Browse files
authored
HIVE-29409: Async request fails to retry on transparent exceptions (#6284)
1 parent c80c721 commit 7b54334

File tree

7 files changed

+252
-25
lines changed

7 files changed

+252
-25
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1530,6 +1530,7 @@ public void dropDatabase(DropDatabaseRequest req) throws TException {
15301530
try {
15311531
while (!resp.isFinished() && !Thread.currentThread().isInterrupted()) {
15321532
resp = client.drop_database_req(req);
1533+
req.setId(resp.getId());
15331534
if (resp.getMessage() != null) {
15341535
LOG.info(resp.getMessage());
15351536
}
@@ -1706,6 +1707,7 @@ public void dropTable(String catName, String dbname, String name, boolean delete
17061707
try {
17071708
while (!resp.isFinished() && !Thread.currentThread().isInterrupted()) {
17081709
resp = client.drop_table_req(dropTableReq);
1710+
dropTableReq.setId(resp.getId());
17091711
if (resp.getMessage() != null) {
17101712
LOG.info(resp.getMessage());
17111713
}

standalone-metastore/metastore-server/pom.xml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
<name>Hive Metastore Server</name>
2424
<properties>
2525
<standalone.metastore.path.to.root>..</standalone.metastore.path.to.root>
26-
<reflections.version>0.10.2</reflections.version>
2726
</properties>
2827
<dependencies>
2928
<dependency>

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1587,6 +1587,10 @@ public AsyncOperationResp drop_database_req(final DropDatabaseRequest req)
15871587
return status.toAsyncOperationResp();
15881588
} catch (Exception e) {
15891589
ex = e;
1590+
// Reset the id of the request in case of RetryingHMSHandler retries
1591+
if (req.getId() != null && req.isAsyncDrop() && !req.isCancel()) {
1592+
req.setId(null);
1593+
}
15901594
throw handleException(e)
15911595
.throwIfInstance(NoSuchObjectException.class, InvalidOperationException.class, MetaException.class)
15921596
.defaultMetaException();
@@ -2429,6 +2433,12 @@ public AsyncOperationResp drop_table_req(DropTableRequest dropReq)
24292433
return status.toAsyncOperationResp();
24302434
} catch (Exception e) {
24312435
ex = e;
2436+
// Here we get an exception, the RetryingHMSHandler might retry the call,
2437+
// need to clear the id from the request, so AbstractRequestHandler can
2438+
// start a new execution other than reuse the cached failed handler.
2439+
if (dropReq.getId() != null && dropReq.isAsyncDrop() && !dropReq.isCancel()) {
2440+
dropReq.setId(null);
2441+
}
24322442
throw handleException(e).throwIfInstance(MetaException.class, NoSuchObjectException.class)
24332443
.convertIfInstance(IOException.class, MetaException.class).defaultMetaException();
24342444
} finally {

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/CreateTableHandler.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -413,27 +413,27 @@ protected void beforeExecute() throws TException, IOException {
413413
String catName = tbl.getCatName();
414414
// Check that constraints have catalog name properly set first
415415
if (CollectionUtils.isNotEmpty(constraints.getPrimaryKeys())
416-
&& !constraints.getPrimaryKeys().get(0).isSetCatName()) {
416+
&& !constraints.getPrimaryKeys().getFirst().isSetCatName()) {
417417
constraints.getPrimaryKeys().forEach(constraint -> constraint.setCatName(catName));
418418
}
419419
if (CollectionUtils.isNotEmpty(constraints.getForeignKeys())
420-
&& !constraints.getForeignKeys().get(0).isSetCatName()) {
420+
&& !constraints.getForeignKeys().getFirst().isSetCatName()) {
421421
constraints.getForeignKeys().forEach(constraint -> constraint.setCatName(catName));
422422
}
423423
if (CollectionUtils.isNotEmpty(constraints.getUniqueConstraints())
424-
&& !constraints.getUniqueConstraints().get(0).isSetCatName()) {
424+
&& !constraints.getUniqueConstraints().getFirst().isSetCatName()) {
425425
constraints.getUniqueConstraints().forEach(constraint -> constraint.setCatName(catName));
426426
}
427427
if (CollectionUtils.isNotEmpty(constraints.getNotNullConstraints())
428-
&& !constraints.getNotNullConstraints().get(0).isSetCatName()) {
428+
&& !constraints.getNotNullConstraints().getFirst().isSetCatName()) {
429429
constraints.getNotNullConstraints().forEach(constraint -> constraint.setCatName(catName));
430430
}
431431
if (CollectionUtils.isNotEmpty(constraints.getDefaultConstraints())
432-
&& !constraints.getDefaultConstraints().get(0).isSetCatName()) {
432+
&& !constraints.getDefaultConstraints().getFirst().isSetCatName()) {
433433
constraints.getDefaultConstraints().forEach(constraint -> constraint.setCatName(catName));
434434
}
435435
if (CollectionUtils.isNotEmpty(constraints.getCheckConstraints())
436-
&& !constraints.getCheckConstraints().get(0).isSetCatName()) {
436+
&& !constraints.getCheckConstraints().getFirst().isSetCatName()) {
437437
constraints.getCheckConstraints().forEach(constraint -> constraint.setCatName(catName));
438438
}
439439
}

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/DropDatabaseHandler.java

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -212,24 +212,7 @@ protected void beforeExecute() throws TException, IOException {
212212
pkgRequest.setDbName(name);
213213
packages = defaultEmptyList(rs.listPackages(pkgRequest));
214214

215-
if (!request.isCascade()) {
216-
if (!tableNames.isEmpty()) {
217-
throw new InvalidOperationException(
218-
"Database " + db.getName() + " is not empty. One or more tables exist.");
219-
}
220-
if (!functions.isEmpty()) {
221-
throw new InvalidOperationException(
222-
"Database " + db.getName() + " is not empty. One or more functions exist.");
223-
}
224-
if (!procedures.isEmpty()) {
225-
throw new InvalidOperationException(
226-
"Database " + db.getName() + " is not empty. One or more stored procedures exist.");
227-
}
228-
if (!packages.isEmpty()) {
229-
throw new InvalidOperationException(
230-
"Database " + db.getName() + " is not empty. One or more packages exist.");
231-
}
232-
}
215+
validateCascadeDrop(tableNames);
233216
Path path = new Path(db.getLocationUri()).getParent();
234217
if (!handler.getWh().isWritable(path)) {
235218
throw new MetaException("Database not dropped since its external warehouse location " + path +
@@ -271,6 +254,27 @@ private void checkFuncPathToCm() {
271254
result.setFunctionCmPaths(funcNeedCmPaths);
272255
}
273256

257+
private void validateCascadeDrop(List<String> tableNames) throws InvalidOperationException {
258+
if (!request.isCascade()) {
259+
if (!tableNames.isEmpty()) {
260+
throw new InvalidOperationException(
261+
"Database " + db.getName() + " is not empty. One or more tables exist.");
262+
}
263+
if (!functions.isEmpty()) {
264+
throw new InvalidOperationException(
265+
"Database " + db.getName() + " is not empty. One or more functions exist.");
266+
}
267+
if (!procedures.isEmpty()) {
268+
throw new InvalidOperationException(
269+
"Database " + db.getName() + " is not empty. One or more stored procedures exist.");
270+
}
271+
if (!packages.isEmpty()) {
272+
throw new InvalidOperationException(
273+
"Database " + db.getName() + " is not empty. One or more packages exist.");
274+
}
275+
}
276+
}
277+
274278
private void checkTablePathPermission(RawStore rs, List<String> tableNames) throws MetaException {
275279
int tableBatchSize = MetastoreConf.getIntVar(handler.getConf(), MetastoreConf.ConfVars.BATCH_RETRIEVE_MAX);
276280
Warehouse wh = handler.getWh();
Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
package org.apache.hadoop.hive.metastore.handler;
20+
21+
import java.nio.file.Paths;
22+
import java.util.HashMap;
23+
import java.util.Map;
24+
25+
import org.apache.hadoop.conf.Configuration;
26+
import org.apache.hadoop.fs.Path;
27+
import org.apache.hadoop.hive.common.TableName;
28+
import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
29+
import org.apache.hadoop.hive.metastore.TableType;
30+
import org.apache.hadoop.hive.metastore.TransactionalMetaStoreEventListener;
31+
import org.apache.hadoop.hive.metastore.annotation.MetastoreCheckinTest;
32+
import org.apache.hadoop.hive.metastore.api.MetaException;
33+
import org.apache.hadoop.hive.metastore.api.NoSuchObjectException;
34+
import org.apache.hadoop.hive.metastore.api.Table;
35+
import org.apache.hadoop.hive.metastore.client.MetaStoreClientTest;
36+
import org.apache.hadoop.hive.metastore.client.builder.DatabaseBuilder;
37+
import org.apache.hadoop.hive.metastore.client.builder.TableBuilder;
38+
import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
39+
import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
40+
import org.apache.hadoop.hive.metastore.events.DropTableEvent;
41+
import org.apache.hadoop.hive.metastore.minihms.AbstractMetaStoreService;
42+
import org.apache.thrift.TException;
43+
import org.datanucleus.exceptions.NucleusException;
44+
import org.junit.After;
45+
import org.junit.Before;
46+
import org.junit.BeforeClass;
47+
import org.junit.Test;
48+
import org.junit.experimental.categories.Category;
49+
import org.junit.runner.RunWith;
50+
import org.junit.runners.Parameterized;
51+
52+
import static org.junit.Assert.assertEquals;
53+
import static org.junit.Assert.assertNotNull;
54+
import static org.junit.Assert.fail;
55+
56+
@RunWith(Parameterized.class)
57+
@Category(MetastoreCheckinTest.class)
58+
public class TestRetryOnException extends MetaStoreClientTest {
59+
protected static final String DB_NAME = "testdroptable";
60+
protected static final String TABLE_NAME = "test_drop_table";
61+
private final String testTempDir =
62+
Paths.get(System.getProperty("java.io.tmpdir"), "testDropTable").toString();
63+
private AbstractMetaStoreService metaStore;
64+
private HiveMetaStoreClient client;
65+
66+
public TestRetryOnException(String name, AbstractMetaStoreService metaStore) {
67+
this.metaStore = metaStore;
68+
}
69+
70+
@BeforeClass
71+
public static void startMetaStores() {
72+
Map<MetastoreConf.ConfVars, String> msConf = new HashMap<>();
73+
Map<String, String> extraConf = new HashMap<>();
74+
extraConf.put("metastore.transactional.event.listeners", AssertExTransactionListener.class.getName());
75+
startMetaStores(msConf, extraConf);
76+
}
77+
78+
@Before
79+
public void setUp() throws Exception {
80+
AssertExTransactionListener.resetTest();
81+
client = metaStore.getClient();
82+
cleanDB();
83+
createDB(DB_NAME);
84+
AssertExTransactionListener.startTest();
85+
}
86+
87+
@Test
88+
public void testRetryOnException() throws Exception {
89+
String location = metaStore.getExternalWarehouseRoot() + "/" + TABLE_NAME;
90+
new TableBuilder()
91+
.setDbName(DB_NAME)
92+
.setTableName(TABLE_NAME)
93+
.addCol("test_id", "int", "test col id")
94+
.addCol("test_value", "string", "test col value")
95+
.addTableParam("param_key", "param_value")
96+
.setType(TableType.EXTERNAL_TABLE.name())
97+
.addTableParam("EXTERNAL", "TRUE")
98+
.addStorageDescriptorParam("sd_param_key", "sd_param_value")
99+
.setSerdeName(TABLE_NAME)
100+
.setStoredAsSubDirectories(false)
101+
.addSerdeParam("serde_param_key", "serde_param_value")
102+
.setLocation(location)
103+
.create(client, metaStore.getConf());
104+
105+
Table table = client.getTable(DB_NAME, TABLE_NAME);
106+
assertNotNull(table);
107+
String tableName = TableName.getQualified(table.getCatName(), table.getDbName(), table.getTableName());
108+
109+
assertEquals(0, AssertExTransactionListener.getCalls(tableName));
110+
client.dropTable(DB_NAME, TABLE_NAME);
111+
assertEquals(AssertExTransactionListener.maxRetries, AssertExTransactionListener.getCalls(tableName));
112+
try {
113+
client.getTable(DB_NAME, TABLE_NAME);
114+
fail("Table " + tableName + " should be dropped");
115+
} catch (NoSuchObjectException nse) {
116+
// expected
117+
}
118+
119+
assertEquals(0, AssertExTransactionListener.getCalls(DB_NAME));
120+
client.dropDatabase(DB_NAME);
121+
assertEquals(AssertExTransactionListener.maxRetries, AssertExTransactionListener.getCalls(tableName));
122+
try {
123+
client.getDatabase(DB_NAME);
124+
fail("Database " + DB_NAME + " should be dropped");
125+
} catch (NoSuchObjectException nse) {
126+
// expected
127+
}
128+
}
129+
130+
public static class AssertExTransactionListener extends TransactionalMetaStoreEventListener {
131+
static Map<String, Integer> calls = new HashMap<>();
132+
static int maxRetries = 5;
133+
static boolean startChecking = false;
134+
135+
public AssertExTransactionListener(Configuration config) {
136+
super(config);
137+
}
138+
139+
@Override
140+
public void onDropTable(DropTableEvent tableEvent) throws MetaException {
141+
Table table = tableEvent.getTable();
142+
String tableName = TableName.getQualified(table.getCatName(), table.getDbName(), table.getTableName());
143+
Integer pre = calls.putIfAbsent(tableName, 1);
144+
if (pre != null) {
145+
calls.put(tableName, pre + 1);
146+
}
147+
// For a drop table call, we should see 5(maxRetries) retries in RetryingHMSHandler
148+
assertException(startChecking && calls.get(tableName) < maxRetries);
149+
}
150+
151+
@Override
152+
public void onDropDatabase(DropDatabaseEvent dbEvent) throws MetaException {
153+
String dbName = dbEvent.getDatabase().getName();
154+
Integer pre = calls.putIfAbsent(dbName, 1);
155+
if (pre != null) {
156+
calls.put(dbName, pre + 1);
157+
}
158+
assertException(startChecking && calls.get(dbName) < maxRetries);
159+
}
160+
161+
private void assertException(boolean throwException) throws MetaException {
162+
if (throwException) {
163+
MetaException exception = new MetaException("An exception for RetryingHMSHandler to retry");
164+
exception.initCause(new NucleusException("Non-fatal exception"));
165+
throw exception;
166+
}
167+
}
168+
169+
static void startTest() {
170+
startChecking = true;
171+
calls.clear();
172+
}
173+
174+
static void resetTest() {
175+
startChecking = false;
176+
calls.clear();
177+
}
178+
179+
static int getCalls(String key) {
180+
return calls.get(key) != null ? calls.get(key) : 0;
181+
}
182+
}
183+
184+
@After
185+
public void tearDown() throws Exception {
186+
try {
187+
if (client != null) {
188+
try {
189+
client.close();
190+
} catch (Exception e) {
191+
}
192+
}
193+
} finally {
194+
client = null;
195+
}
196+
197+
Path path = new Path(testTempDir);
198+
path.getFileSystem(metaStore.getConf()).delete(path, true);
199+
}
200+
201+
protected void cleanDB() throws Exception{
202+
client.dropDatabase(DB_NAME, true, true, true);
203+
metaStore.cleanWarehouseDirs();
204+
}
205+
206+
protected void createDB(String dbName) throws TException {
207+
new DatabaseBuilder().
208+
setName(dbName).
209+
create(client, metaStore.getConf());
210+
}
211+
}

standalone-metastore/pom.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
<protobuf.version>3.25.5</protobuf.version>
102102
<protoc-jar-maven-plugin.version>3.11.4</protoc-jar-maven-plugin.version>
103103
<protoc.path>${env.PROTOC_PATH}</protoc.path>
104+
<reflections.version>0.10.2</reflections.version>
104105
<io.grpc.version>1.72.0</io.grpc.version>
105106
<sqlline.version>1.9.0</sqlline.version>
106107
<netty.version>4.1.127.Final</netty.version>

0 commit comments

Comments
 (0)