From 60e09d6062e50f5d14fd2b554ca3e764733fcca5 Mon Sep 17 00:00:00 2001 From: Rui Li Date: Fri, 15 Mar 2024 22:45:47 +0800 Subject: [PATCH 1/3] HIVE-28121: Use direct SQL for transactional altering table parameter --- .../hive/metastore/HiveAlterHandler.java | 26 ++++++++------ .../hadoop/hive/metastore/ObjectStore.java | 34 +++++++++++++++++++ .../hadoop/hive/metastore/RawStore.java | 10 ++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java index 68b522ff7471..f4012d5c541c 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java @@ -57,7 +57,6 @@ import org.apache.hadoop.hive.metastore.api.Table; import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants; -import javax.jdo.Constants; import java.io.IOException; import java.net.URI; import java.util.ArrayList; @@ -184,12 +183,7 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam String expectedValue = environmentContext != null && environmentContext.getProperties() != null ? environmentContext.getProperties().get(hive_metastoreConstants.EXPECTED_PARAMETER_VALUE) : null; - if (expectedKey != null) { - // If we have to check the expected state of the table we have to prevent nonrepeatable reads. - msdb.openTransaction(Constants.TX_REPEATABLE_READ); - } else { - msdb.openTransaction(); - } + msdb.openTransaction(); // get old table // Note: we don't verify stats here; it's done below in alterTableUpdateTableColumnStats. olddb = msdb.getDatabase(catName, dbname); @@ -199,10 +193,20 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam TableName.getQualified(catName, dbname, name) + " doesn't exist"); } - if (expectedKey != null && expectedValue != null - && !expectedValue.equals(oldt.getParameters().get(expectedKey))) { - throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '" - + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'"); + if (expectedKey != null && expectedValue != null) { + String newValue = newt.getParameters().get(expectedKey); + if (newValue == null) { + throw new MetaException(String.format("New value for expected key %s is not set", expectedKey)); + } + if (!expectedValue.equals(oldt.getParameters().get(expectedKey))) { + throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '" + + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'"); + } + int affectedRows = msdb.updateParameterWithExpectedValue(oldt, expectedKey, expectedValue, newValue); + if (affectedRows != 1) { + throw new MetaException(String.format( + "The table has been modified. Updating expected key %s affects %d rows", expectedKey, affectedRows)); + } } validateTableChangesOnReplSource(olddb, oldt, newt, environmentContext); diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java index a810c9cc695f..90d219cdda58 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java @@ -624,6 +624,40 @@ public boolean openTransaction(String isolationLevel) { return result; } + @Override + public int updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) + throws MetaException { + String dml = String.format( + "UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = '%s' " + + "WHERE \"TBL_ID\" = %d AND \"PARAM_KEY\" = '%s' AND \"PARAM_VALUE\" = '%s'", + newValue, + table.getId(), + key, + expectedValue + ); + JDOConnection jdoConn = null; + try { + openTransaction(); + jdoConn = pm.getDataStoreConnection(); + Object nativeConn = jdoConn.getNativeConnection(); + if (!(nativeConn instanceof Connection)) { + throw new MetaException("Unexpected JDO native connection " + nativeConn.getClass().getName()); + } + Connection dbConn = (Connection) nativeConn; + try (Statement statement = dbConn.createStatement()) { + int affectedRows = statement.executeUpdate(dml); + commitTransaction(); + return affectedRows; + } + } catch (SQLException e) { + throw new JDOException("Failed to execute direct SQL", e); + } finally { + if (jdoConn != null) { + jdoConn.close(); + } + } + } + /** * if this is the commit of the first open call then an actual commit is * called. diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java index 5dead2d425da..809103161bee 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java @@ -2317,4 +2317,14 @@ default PropertyStore getPropertyStore() { return null; } + /** + * Updates a given table parameter with expected value. + * + * @return the number of rows updated + */ + default int updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) + throws MetaException { + throw new UnsupportedOperationException("This Store doesn't support updating table parameter with expected value"); + } + } From a0a0890b42898fcdc6f796791c94414d6150f863 Mon Sep 17 00:00:00 2001 From: Rui Li Date: Thu, 28 Mar 2024 19:18:02 +0800 Subject: [PATCH 2/3] use MetaStoreDirectSql --- .../hive/metastore/HiveAlterHandler.java | 2 +- .../hive/metastore/MetaStoreDirectSql.java | 10 +++++ .../hadoop/hive/metastore/ObjectStore.java | 45 +++++++------------ .../hadoop/hive/metastore/RawStore.java | 4 +- .../TestTablesCreateDropAlterTruncate.java | 7 +++ 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java index f4012d5c541c..19fff49eb566 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java @@ -202,7 +202,7 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is '" + oldt.getParameters().get(expectedKey) + "'. The expected was value was '" + expectedValue + "'"); } - int affectedRows = msdb.updateParameterWithExpectedValue(oldt, expectedKey, expectedValue, newValue); + long affectedRows = msdb.updateParameterWithExpectedValue(oldt, expectedKey, expectedValue, newValue); if (affectedRows != 1) { throw new MetaException(String.format( "The table has been modified. Updating expected key %s affects %d rows", expectedKey, affectedRows)); diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java index 4a1f2ca8db29..0f264818b942 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java @@ -53,6 +53,7 @@ import javax.jdo.Transaction; import javax.jdo.datastore.JDOConnection; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.conf.Configuration; @@ -3298,4 +3299,13 @@ private List getFunctionIds(String catName) throws MetaException { return result; } } + + long updateTableParam(Table table, String key, String expectedValue, String newValue) { + String statement = TxnUtils.createUpdatePreparedStmt( + "\"TABLE_PARAMS\"", + ImmutableList.of("\"PARAM_VALUE\""), + ImmutableList.of("\"TBL_ID\"", "\"PARAM_KEY\"", "\"PARAM_VALUE\"")); + Query query = pm.newQuery("javax.jdo.query.SQL", statement); + return (long) query.executeWithArray(newValue, table.getId(), key, expectedValue); + } } diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java index 90d219cdda58..ce4e96927b7a 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java @@ -625,37 +625,26 @@ public boolean openTransaction(String isolationLevel) { } @Override - public int updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) - throws MetaException { - String dml = String.format( - "UPDATE \"TABLE_PARAMS\" SET \"PARAM_VALUE\" = '%s' " + - "WHERE \"TBL_ID\" = %d AND \"PARAM_KEY\" = '%s' AND \"PARAM_VALUE\" = '%s'", - newValue, - table.getId(), - key, - expectedValue - ); - JDOConnection jdoConn = null; - try { - openTransaction(); - jdoConn = pm.getDataStoreConnection(); - Object nativeConn = jdoConn.getNativeConnection(); - if (!(nativeConn instanceof Connection)) { - throw new MetaException("Unexpected JDO native connection " + nativeConn.getClass().getName()); + public long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) + throws MetaException, NoSuchObjectException { + return new GetHelper(table.getCatName(), table.getDbName(), table.getTableName(), true, false) { + + @Override + protected String describeResult() { + return "Affected rows"; } - Connection dbConn = (Connection) nativeConn; - try (Statement statement = dbConn.createStatement()) { - int affectedRows = statement.executeUpdate(dml); - commitTransaction(); - return affectedRows; + + @Override + protected Long getSqlResult(GetHelper ctx) throws MetaException { + return directSql.updateTableParam(table, key, expectedValue, newValue); } - } catch (SQLException e) { - throw new JDOException("Failed to execute direct SQL", e); - } finally { - if (jdoConn != null) { - jdoConn.close(); + + @Override + protected Long getJdoResult(GetHelper ctx) throws MetaException, NoSuchObjectException, InvalidObjectException { + throw new UnsupportedOperationException( + "Cannot update parameter with JDO, make sure direct SQL is enabled"); } - } + }.run(false); } /** diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java index 809103161bee..5ab2907f5baa 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java @@ -2322,8 +2322,8 @@ default PropertyStore getPropertyStore() { * * @return the number of rows updated */ - default int updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) - throws MetaException { + default long updateParameterWithExpectedValue(Table table, String key, String expectedValue, String newValue) + throws MetaException, NoSuchObjectException { throw new UnsupportedOperationException("This Store doesn't support updating table parameter with expected value"); } diff --git a/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java b/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java index cf4fc482808e..6e0616713f89 100644 --- a/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java +++ b/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java @@ -63,6 +63,7 @@ import org.apache.thrift.protocol.TProtocolException; import org.junit.After; import org.junit.Assert; +import org.junit.Assume; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; @@ -1196,6 +1197,8 @@ public void testAlterTableAlreadyExists() throws Exception { @Test public void testAlterTableExpectedPropertyMatch() throws Exception { + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL)); + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL)); Table originalTable = testTables[0]; EnvironmentContext context = new EnvironmentContext(); @@ -1209,6 +1212,8 @@ public void testAlterTableExpectedPropertyMatch() throws Exception { @Test(expected = MetaException.class) public void testAlterTableExpectedPropertyDifferent() throws Exception { + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL)); + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL)); Table originalTable = testTables[0]; EnvironmentContext context = new EnvironmentContext(); @@ -1228,6 +1233,8 @@ public void testAlterTableExpectedPropertyDifferent() throws Exception { */ @Test public void testAlterTableExpectedPropertyConcurrent() throws Exception { + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL)); + Assume.assumeTrue(MetastoreConf.getBoolVar(metaStore.getConf(), ConfVars.TRY_DIRECT_SQL_DDL)); Table originalTable = testTables[0]; originalTable.getParameters().put("snapshot", "0"); From 54a62df293e4d284cf430c209cf94695253bb04e Mon Sep 17 00:00:00 2001 From: Rui Li Date: Mon, 1 Apr 2024 14:30:12 +0800 Subject: [PATCH 3/3] change exception msg --- .../org/apache/hadoop/hive/metastore/HiveAlterHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java index 19fff49eb566..1d930eea7d42 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java @@ -204,8 +204,8 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam } long affectedRows = msdb.updateParameterWithExpectedValue(oldt, expectedKey, expectedValue, newValue); if (affectedRows != 1) { - throw new MetaException(String.format( - "The table has been modified. Updating expected key %s affects %d rows", expectedKey, affectedRows)); + // make sure concurrent modification exception messages have the same prefix + throw new MetaException("The table has been modified. The parameter value for key '" + expectedKey + "' is different"); } }