[
https://issues.apache.org/jira/browse/HIVE-26793?focusedWorklogId=830349&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-830349
]
ASF GitHub Bot logged work on HIVE-26793:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 01/Dec/22 14:09
Start Date: 01/Dec/22 14:09
Worklog Time Spent: 10m
Work Description: veghlaci05 commented on code in PR #3822:
URL: https://github.com/apache/hive/pull/3822#discussion_r1036973940
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSMetricsListener.java:
##########
@@ -99,15 +103,22 @@ public void onAddPartition(AddPartitionEvent
partitionEvent) throws MetaExceptio
public void onAllocWriteId(AllocWriteIdEvent allocWriteIdEvent, Connection
dbConn, SQLGenerator sqlGenerator) throws MetaException {
if (MetastoreConf.getBoolVar(getConf(),
MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Table table = getTable(allocWriteIdEvent);
+ Database db;
+ try {
+ db =
HMSHandler.getMSForConf(getConf()).getDatabase(getDefaultCatalog(getConf()),
table.getDbName());
Review Comment:
You should use table.getCatName() if present, and please make this change in
org.apache.hadoop.hive.ql.txn.compactor.Initiator#resolveDatabase too. I just
realized this could be an issue if the DB is not in the default catalog.
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorOnTezTest.java:
##########
@@ -259,6 +259,16 @@ void createDb(String dbName, String poolName) throws
Exception {
executeStatementOnDriver("create database " + dbName + " WITH
DBPROPERTIES('hive.compactor.worker.pool'='" + poolName + "')", driver);
}
+ void createDb(String dbName, boolean noAutoCompact) throws Exception {
+ executeStatementOnDriver("drop database if exists " + dbName + "
cascade", driver);
+ executeStatementOnDriver("create database " + dbName + " WITH
DBPROPERTIES('no_auto_compaction'='" + noAutoCompact + "')", driver);
+ }
+
+ void createDbWithLowerCaseProps(String dbName, boolean noAutoCompact)
throws Exception {
Review Comment:
you could add a third param like boolean upperCaseProperty to createDb and
remove this function
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -1143,15 +1143,24 @@ public static TableName getTableNameFor(Table table) {
/**
* Because TABLE_NO_AUTO_COMPACT was originally assumed to be
NO_AUTO_COMPACT and then was moved
* to no_auto_compact, we need to check it in both cases.
+ * Check the database level no_auto_compact , if present it is given
priority else table level no_auto_compact is considered.
*/
- public static boolean isNoAutoCompactSet(Map<String, String> parameters) {
- String noAutoCompact =
- parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
- if (noAutoCompact == null) {
- noAutoCompact =
-
parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase());
- }
- return noAutoCompact != null && noAutoCompact.equalsIgnoreCase("true");
+ public static boolean isNoAutoCompactSet(Map<String, String> dbParameters,
Map<String, String> parameters) {
+ String dbNoAutoCompact =
dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT) == null ?
+
dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase()) :
+ dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
+ if (dbNoAutoCompact == null) {
+ LOG.info("Using table configuration '" +
hive_metastoreConstants.TABLE_NO_AUTO_COMPACT + "' for compaction");
Review Comment:
This will exeecuted frequently, should be log.DEBUG to avoid spamming the
logs
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.ql.txn.compactor;
+
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.*;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class TestInitiator2 extends CompactorOnTezTest {
+ @Test
+ public void dbNoAutoCompactSetTrueUpperCase() throws Exception {
Review Comment:
The only real difference with the other method is the
`testDataProvider.createDbWithLowerCaseProps` call and the expectation whether
initiator created a request or not. Consider creating a private method which
takes the following arguments: boolean noAutoCompaction, boolean upperCaseProp.
This should be called from the test methods instead of duplicating the code.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSMetricsListener.java:
##########
@@ -99,15 +103,22 @@ public void onAddPartition(AddPartitionEvent
partitionEvent) throws MetaExceptio
public void onAllocWriteId(AllocWriteIdEvent allocWriteIdEvent, Connection
dbConn, SQLGenerator sqlGenerator) throws MetaException {
if (MetastoreConf.getBoolVar(getConf(),
MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Table table = getTable(allocWriteIdEvent);
+ Database db;
+ try {
+ db =
HMSHandler.getMSForConf(getConf()).getDatabase(getDefaultCatalog(getConf()),
table.getDbName());
+ } catch (NoSuchObjectException e) {
+ LOGGER.error("Unable to find database " + table.getDbName() + ", " +
e.getMessage());
+ throw new MetaException(String.valueOf(e));
+ }
// In the case of CTAS, the table is created after write ids are
allocated, so we'll skip metrics collection.
- if (table != null &&
MetaStoreUtils.isNoAutoCompactSet(table.getParameters())) {
+ if (table != null &&
MetaStoreUtils.isNoAutoCompactSet(db.getParameters(), table.getParameters())) {
Review Comment:
At this point table is never null as you are already called
table.getDbName() a few lines above. This check should be moved up, if table is
null an NPE will be thrown even in the catch block.
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics2.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.ql.txn.compactor;
+
+import org.apache.hadoop.hive.metastore.HMSMetricsListener;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.metrics.Metrics;
+import org.apache.hadoop.hive.metastore.metrics.MetricsConstants;
+import org.apache.hadoop.hive.metastore.txn.TxnUtils;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+
+public class TestCompactionMetrics2 extends CompactorOnTezTest {
Review Comment:
You should not mix up the test classes. The tests should be either in
`hive-exec` module and extend `CompactorTest` , or in `hive-it-unit` and extend
`CompactorOnTezTest` or `TestCompactorBase`. Altough the preferred place of
writing new tests is `hive-it-unit`, if it's easier to write the tests in
`hive-exec`, feel free to do it. But you should choose either of the test
infrastructure and build your tests only upon that one.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSMetricsListener.java:
##########
@@ -99,15 +103,22 @@ public void onAddPartition(AddPartitionEvent
partitionEvent) throws MetaExceptio
public void onAllocWriteId(AllocWriteIdEvent allocWriteIdEvent, Connection
dbConn, SQLGenerator sqlGenerator) throws MetaException {
if (MetastoreConf.getBoolVar(getConf(),
MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Table table = getTable(allocWriteIdEvent);
+ Database db;
+ try {
+ db =
HMSHandler.getMSForConf(getConf()).getDatabase(getDefaultCatalog(getConf()),
table.getDbName());
+ } catch (NoSuchObjectException e) {
+ LOGGER.error("Unable to find database " + table.getDbName() + ", " +
e.getMessage());
+ throw new MetaException(String.valueOf(e));
+ }
// In the case of CTAS, the table is created after write ids are
allocated, so we'll skip metrics collection.
- if (table != null &&
MetaStoreUtils.isNoAutoCompactSet(table.getParameters())) {
+ if (table != null &&
MetaStoreUtils.isNoAutoCompactSet(db.getParameters(), table.getParameters())) {
int noAutoCompactSet =
Review Comment:
Please rename this variable to sth like `writeThreshold`, as its current
name is really confusing. It holds the threshold, not a boolean value.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -1143,15 +1143,24 @@ public static TableName getTableNameFor(Table table) {
/**
* Because TABLE_NO_AUTO_COMPACT was originally assumed to be
NO_AUTO_COMPACT and then was moved
* to no_auto_compact, we need to check it in both cases.
+ * Check the database level no_auto_compact , if present it is given
priority else table level no_auto_compact is considered.
*/
- public static boolean isNoAutoCompactSet(Map<String, String> parameters) {
- String noAutoCompact =
- parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
- if (noAutoCompact == null) {
- noAutoCompact =
-
parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase());
- }
- return noAutoCompact != null && noAutoCompact.equalsIgnoreCase("true");
+ public static boolean isNoAutoCompactSet(Map<String, String> dbParameters,
Map<String, String> parameters) {
+ String dbNoAutoCompact =
dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT) == null ?
+
dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase()) :
+ dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
+ if (dbNoAutoCompact == null) {
+ LOG.info("Using table configuration '" +
hive_metastoreConstants.TABLE_NO_AUTO_COMPACT + "' for compaction");
+ String noAutoCompact =
+ parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
+ if (noAutoCompact == null) {
+ noAutoCompact =
+
parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase());
+ }
+ return noAutoCompact != null && noAutoCompact.equalsIgnoreCase("true");
+ }
+ LOG.info("Using database configuration '" +
hive_metastoreConstants.TABLE_NO_AUTO_COMPACT + "' for compaction");
Review Comment:
This will exeecuted frequently, should be log.DEBUG to avoid spamming the
logs
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -1143,15 +1143,24 @@ public static TableName getTableNameFor(Table table) {
/**
* Because TABLE_NO_AUTO_COMPACT was originally assumed to be
NO_AUTO_COMPACT and then was moved
* to no_auto_compact, we need to check it in both cases.
+ * Check the database level no_auto_compact , if present it is given
priority else table level no_auto_compact is considered.
*/
- public static boolean isNoAutoCompactSet(Map<String, String> parameters) {
- String noAutoCompact =
- parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
- if (noAutoCompact == null) {
- noAutoCompact =
-
parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase());
- }
- return noAutoCompact != null && noAutoCompact.equalsIgnoreCase("true");
+ public static boolean isNoAutoCompactSet(Map<String, String> dbParameters,
Map<String, String> parameters) {
+ String dbNoAutoCompact =
dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT) == null ?
+
dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase()) :
+ dbParameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
+ if (dbNoAutoCompact == null) {
+ LOG.info("Using table configuration '" +
hive_metastoreConstants.TABLE_NO_AUTO_COMPACT + "' for compaction");
+ String noAutoCompact =
+ parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT);
+ if (noAutoCompact == null) {
+ noAutoCompact =
+
parameters.get(hive_metastoreConstants.TABLE_NO_AUTO_COMPACT.toUpperCase());
+ }
Review Comment:
Either use the ternary operator here or use if above for DB parameter. Since
you are doing the same logic with only a different parameter you can even
create a small private helper method for it.
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactorBase.java:
##########
@@ -180,8 +180,13 @@ private void createTestDataFile(String filename, String[]
lines) throws IOExcept
writer.close();
}
}
-
}
-
+ // Sub Class of CompactorTest which is the super class of all compactor test
modules
Review Comment:
Please remove this class
Issue Time Tracking
-------------------
Worklog Id: (was: 830349)
Time Spent: 0.5h (was: 20m)
> Create a new configuration to override "no compaction" for tables
> -----------------------------------------------------------------
>
> Key: HIVE-26793
> URL: https://issues.apache.org/jira/browse/HIVE-26793
> Project: Hive
> Issue Type: Improvement
> Reporter: Kokila N
> Assignee: Kokila N
> Priority: Major
> Labels: pull-request-available
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently a simple user can create a table with
> {color:#6a8759}no_auto_compaction=true{color} table property and create an
> aborted write transaction writing to this table. This way a malicious user
> can prevent cleaning up data for the aborted transaction, creating
> performance degradation.
> This configuration should be allowed to overridden on a database level:
> adding {color:#6a8759}no_auto_compaction=false{color} should override the
> table level setting forcing the initiator to schedule compaction for all
> tables.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
