----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69704/#review212117 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java Lines 2539 (patched) <https://reviews.apache.org/r/69704/#comment297735> JavaDoc ql/src/java/org/apache/hadoop/hive/ql/io/HdfsUtils.java Lines 97 (patched) <https://reviews.apache.org/r/69704/#comment297736> JavaDoc ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java Lines 83 (patched) <https://reviews.apache.org/r/69704/#comment297728> Since you only have a single HMS connection (I assume this is what this locks is protecting), wouldn't it be better to get the table/partition path before parallelizing the work that can actually be parallelized? This way you fork threads and then synch them immediately. ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java Lines 140 (patched) <https://reviews.apache.org/r/69704/#comment297730> I'm not sure this achieves what the commnet says. For normal clean (as we had before) you may have > 1 compaction_queue entry in ready for cleaning. You should not have > 1 entry in Working state for the same partition, you may have > 1 entry in ready-for-cleaning since you have more workers than Cleaners. It's perhaps made even worse by the new "table level" clean. I think you are right to worry about this though. I'll make a more detail comment on the Jira shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java Lines 797 (patched) <https://reviews.apache.org/r/69704/#comment297734> Why is this needed? It should have some JavaDoc standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java Line 91 (original), 91 (patched) <https://reviews.apache.org/r/69704/#comment297733> I don't think this is right. You are now counting aborted txns by type, so that you need > maxAborted aborted Inserts or > maxAborted aborted Updates, etc to trigger compaction rather than ( > maxAborted of (aborted inserts + updates+ deletes) standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Line 1228 (original), 1231 (patched) <https://reviews.apache.org/r/69704/#comment297737> exclude 'p' type here - Eugene Koifman On Jan. 16, 2019, 10:08 a.m., Jaume Marhuenda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69704/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2019, 10:08 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > Make sure transaction get cleaned if they are aborted before addPartitions is > called > > > Diffs > ----- > > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactor.java > dc7b2877bf > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 5dbf634825 > ql/src/java/org/apache/hadoop/hive/ql/io/HdfsUtils.java 3482cfce36 > ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java 06b0209aa0 > ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java > a0df82cb20 > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > 5e085f84af > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java > b6f70ebe63 > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java > c569b242ae > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/AddDynamicPartitions.java > 9c33229270 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/AlterPartitionsRequest.java > f7d9ed2e2e > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ClearFileMetadataRequest.java > f4e3d6bd71 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ClientCapabilities.java > 2b394449a3 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/CompactionInfoStruct.java > 4aee45ce5f > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/CompactionType.java > 7450b27cf3 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/CreationMetadata.java > 9595a5dc10 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/FindSchemasByColsResp.java > 42073db544 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/FireEventRequest.java > dd6658d636 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetAllFunctionsResponse.java > 68146e4561 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetFileMetadataByExprRequest.java > ee535a0c80 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetFileMetadataByExprResult.java > 71e92b6c03 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetFileMetadataRequest.java > 0ea6ef5fb3 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetFileMetadataResult.java > 759b495bf6 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsFilterSpec.java > b5a2b68efd > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsProjectionSpec.java > e6c9c06beb > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsRequest.java > 7ec107ea6c > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsResponse.java > faac848991 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetTablesRequest.java > da361572e5 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetTablesResult.java > b3cfc88b34 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/InsertEventRequestData.java > f1ba64348e > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/NotificationEventRequest.java > 288c365950 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/NotificationEventResponse.java > b86f038c1e > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PutFileMetadataRequest.java > 5cbfe64945 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/RenamePartitionRequest.java > ea4cc16af5 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/SchemaVersion.java > b87f65f524 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ShowCompactResponse.java > 2a7b3eba2a > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > 1bdbbbf363 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WMFullResourcePlan.java > f92e23ea20 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WMGetAllResourcePlanResponse.java > cd20b15be4 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WMGetTriggersForResourePlanResponse.java > 0fc76b9ffc > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WMValidateResourcePlanResponse.java > deb1569bb1 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/WriteNotificationLogRequest.java > 57f50b7dc8 > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php > b94dd25b84 > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php > 13e287e352 > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 37db81ff73 > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ttypes.py > 8f149d1d6e > > standalone-metastore/metastore-common/src/gen/thrift/gen-rb/hive_metastore_types.rb > 9e5f0860f2 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > be1f8c7849 > standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift > 9576f8775a > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionInfo.java > ea70503988 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java > 8253ccb9c9 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > 91a9ab4053 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java > 898a94dcd6 > > > Diff: https://reviews.apache.org/r/69704/diff/2/ > > > Testing > ------- > > Planning to manually test it using the hive-spark connector, which is where > this bug was discovered. > > > Thanks, > > Jaume Marhuenda > >
