Re: Review Request 69704: HIVE-21052

2019-01-28 Thread Jaume Marhuenda 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69704/
---

(Updated Jan. 28, 2019, 10:32 p.m.)


Review request for hive.


Repository: hive-git


Description
---

Make sure transaction get cleaned if they are aborted before addPartitions is 
called


Diffs (updated)
-

  
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/CleanerExecutorService.java 
PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java a0df82cb20 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/NonReentrantReadWriteLock.java
 PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
5e085f84af 
  
ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCleanerExecutorService.java
 PRE-CREATION 
  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/CompactionType.java
 7450b27cf3 
  
standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php
 13e287e352 
  
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/4/

Changes: https://reviews.apache.org/r/69704/diff/3-4/


Testing
---

Planning to manually test it using the hive-spark connector, which is where 
this bug was discovered.


Thanks,

Jaume Marhuenda

Re: Review Request 69704: HIVE-21052

2019-01-23 Thread Jaume Marhuenda 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69704/
---

(Updated Jan. 23, 2019, 9:08 p.m.)


Review request for hive.


Repository: hive-git


Description
---

Make sure transaction get cleaned if they are aborted before addPartitions is 
called


Diffs (updated)
-

  
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/CompactionType.java
 7450b27cf3 
  
standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php
 13e287e352 
  
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/3/

Changes: https://reviews.apache.org/r/69704/diff/2-3/


Testing
---

Planning to manually test it using the hive-spark connector, which is where 
this bug was discovered.


Thanks,

Jaume Marhuenda

Re: Review Request 69704: HIVE-21052

2019-01-17 Thread Eugene Koifman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69704/#review212120
---




ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java
Line 129 (original), 129 (patched)


This doesn't check 'p' type compactions so you could enqueue multiple ones 
for the same table, but see my Jira comments.



standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/CompactionInfoStruct.java
Lines 53 (patched)


why is this needed?  when is the writeId list ever get passed over the wire?


- 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 
>   
>

Re: Review Request 69704: HIVE-21052

2019-01-17 Thread Eugene Koifman 

---
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)


JavaDoc



ql/src/java/org/apache/hadoop/hive/ql/io/HdfsUtils.java
Lines 97 (patched)


JavaDoc



ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
Lines 83 (patched)


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)


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)


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)


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)


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 
>   
>

Re: Review Request 69704: HIVE-21052

2019-01-16 Thread Jaume Marhuenda 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69704/
---

(Updated Jan. 16, 2019, 6:08 p.m.)


Review request for hive.


Repository: hive-git


Description
---

Make sure transaction get cleaned if they are aborted before addPartitions is 
called


Diffs (updated)
-

  
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