You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/03/09 09:24:03 UTC
[GitHub] [hive] kgyrtkirk commented on a change in pull request #2044: HIVE-24853. HMS leaks queries in case of timeout.
kgyrtkirk commented on a change in pull request #2044:
URL: https://github.com/apache/hive/pull/2044#discussion_r589355466
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
##########
@@ -390,13 +394,16 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam
part.getValues(), oldCols, oldt, part, null, null);
assert (colStats.isEmpty());
if (cascade) {
Review comment:
I think it will enough to have 1 call in this method - either here above the if or keep the one after the block
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java
##########
@@ -138,23 +138,30 @@ static void timingTrace(boolean doTrace, String queryText, long start, long quer
List<Object[]> list = ensureList(result);
Iterator<Object[]> iter = list.iterator();
Object[] fields = null;
- for (Map.Entry<Long, T> entry : tree.entrySet()) {
- if (fields == null && !iter.hasNext()) break;
- long id = entry.getKey();
- while (fields != null || iter.hasNext()) {
- if (fields == null) {
- fields = iter.next();
+ int rv = 0;
+ try {
+ for (Map.Entry<Long, T> entry : tree.entrySet()) {
+ if (fields == null && !iter.hasNext())
+ break;
+ long id = entry.getKey();
+ while (fields != null || iter.hasNext()) {
+ if (fields == null) {
+ fields = iter.next();
+ }
+ long nestedId = extractSqlLong(fields[keyIndex]);
+ if (nestedId < id)
Review comment:
please follow the hive formatter braces (`{}`) are mandatory for even 1 line if blocks as well
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
##########
@@ -355,8 +357,10 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam
writeIdList, newt.getWriteId());
}
} else {
+ Deadline.checkTimeout();
Review comment:
I think 1 check should be enough in every loop
a few lines above the `for (Entry<Partition, ColumnStatistics> partColStats : columnStatsNeedUpdated.entries()) {`
doesnt have any checks
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1783,11 +1783,15 @@ private long partsFoundForPartitions(
MetastoreDirectSqlUtils.timingTrace(doTrace, queryText, start, end);
List<Object[]> list = MetastoreDirectSqlUtils.ensureList(qResult);
List<ColumnStatisticsObj> colStats = new ArrayList<ColumnStatisticsObj>(list.size());
- for (Object[] row : list) {
- colStats.add(prepareCSObjWithAdjustedNDV(row, 0, useDensityFunctionForNDVEstimation, ndvTuner));
- Deadline.checkTimeout();
+ try {
Review comment:
instead of try-finally ; can't we switch to try-with-resources for the query object ?
note that for example `ensureList` may also throw an exception in some cases - and in that particular case the query will not be closed...
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
##########
@@ -559,11 +567,14 @@ public Partition alterPartition(RawStore msdb, Warehouse wh, String catName, Str
// PartitionView does not have SD. We do not need update its column stats
if (oldPart.getSd() != null) {
+ Deadline.checkTimeout();
Review comment:
I think we should be ok to only check this once in every for loop
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1814,22 +1818,26 @@ private long partsFoundForPartitions(
List<String> noExtraColumnNames = new ArrayList<String>();
Map<String, String[]> extraColumnNameTypeParts = new HashMap<String, String[]>();
List<Object[]> list = MetastoreDirectSqlUtils.ensureList(qResult);
- for (Object[] row : list) {
- String colName = (String) row[0];
- String colType = (String) row[1];
- // Extrapolation is not needed for this column if
- // count(\"PARTITION_NAME\")==partNames.size()
- // Or, extrapolation is not possible for this column if
- // count(\"PARTITION_NAME\")<2
- Long count = MetastoreDirectSqlUtils.extractSqlLong(row[2]);
- if (count == partNames.size() || count < 2) {
- noExtraColumnNames.add(colName);
- } else {
- extraColumnNameTypeParts.put(colName, new String[] { colType, String.valueOf(count) });
+ try {
+ for (Object[] row : list) {
+ String colName = (String) row[0];
Review comment:
try-with-resources for `query`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org