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
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
