Re: Review Request 72532: HIVE-23495 AcidUtils.getAcidState cleanup

[Peter Varga via Review Board]

> On June 5, 2020, 2:04 p.m., Karen Coppage wrote:
> > LGTM, a few minor suggestions.
> > (Non-binding)

Thanks for the review.


> On June 5, 2020, 2:04 p.m., Karen Coppage wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
> > Line 1411 (original)
> > 
> >
> > Are these originals not needed, or collected elsewhere?

This one is bothering me, these lines were added when the snapshot way was 
introduced, but I do not see why. When we calculated the AcidState without the 
snapshot these files were not added to the originals list. It is explicitly 
there few lines above, that if we have a base we consider every original files 
as obsolete. The 
TestTxnCommandsForMmTable#testInsertOverwriteForPartitionedMmTable breaks for 
example if these files are added to the list. After an insert-overwrite to a mm 
table and calling the major compaction, the compaction will create a new base 
dir, not leaving the perfectly fine base dir generated by the insert overwrite. 
I did not dig into the compaction to see why the original files are triggering 
it, but I do not think these files needed in the original list.


> On June 5, 2020, 2:04 p.m., Karen Coppage wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
> > Lines 1438 (patched)
> > 
> >
> > might want to add a check + error message before the while loop in case 
> > someone added a file that has no delta or base dir in its path. you never 
> > know what people are capable of :)

This is only looping if we are inside a delta or a base directory in some 
depth, but not directly in the folder. If someone adds a random dir, we will 
just consider it an originalDir and do not enter in the loop.


> On June 5, 2020, 2:04 p.m., Karen Coppage wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
> > Lines 1441 (patched)
> > 
> >
> > nit: typo

fixed


> On June 5, 2020, 2:04 p.m., Karen Coppage wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
> > Lines 1442 (patched)
> > 
> >
> > nit: typo

fixed


> On June 5, 2020, 2:04 p.m., Karen Coppage wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java
> > Line 1937 (original), 1880 (patched)
> > 
> >
> > Does this need to be public?

fixed


- Peter


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


On June 8, 2020, 10:58 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72532/
> ---
> 
> (Updated June 8, 2020, 10:58 a.m.)
> 
> 
> Review request for hive, Karen Coppage, Marta Kuczora, and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> since HIVE-21225 there are two redundant implementation of the 
> AcidUtils.getAcidState.
> 
> The previous implementation (without the recursive listing) can be removed.
> 
> Also the performance can be improved, by removing unnecessary fileStatus 
> calls.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 635ed3149c 
>   ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java ca234cfb37 
>   ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 1059cb227f 
>   ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcRawRecordMerger.java 
> 16c915959c 
>   
> ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java
>  598220b0c4 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java 2a15913f9f 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
> 4e5d5b003b 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 7913295380 
>   
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MinorQueryCompactor.java 
> d83a50f555 
>   
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMajorQueryCompactor.java
>  5e11d8d2d8 
>   
> ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMinorQueryCompactor.java
>  1bdec7df2d 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java 75941b3f33 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 337f469d1a 
>   ql/src/test/org/apache/hadoop/hive/ql/io/TestAcidUtils.java f351f04b08 
>   ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java 
> e4440e9136 
>   ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcRawRecordMerger.java 

Re: Review Request 72532: HIVE-23495 AcidUtils.getAcidState cleanup

[Peter Varga via Review Board]

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

(Updated June 8, 2020, 10:58 a.m.)


Review request for hive, Karen Coppage, Marta Kuczora, and Peter Vary.


Repository: hive-git


Description
---

since HIVE-21225 there are two redundant implementation of the 
AcidUtils.getAcidState.

The previous implementation (without the recursive listing) can be removed.

Also the performance can be improved, by removing unnecessary fileStatus calls.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 635ed3149c 
  ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java ca234cfb37 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 1059cb227f 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcRawRecordMerger.java 
16c915959c 
  
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java
 598220b0c4 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java 2a15913f9f 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
4e5d5b003b 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 7913295380 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MinorQueryCompactor.java 
d83a50f555 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMajorQueryCompactor.java 
5e11d8d2d8 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMinorQueryCompactor.java 
1bdec7df2d 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java 75941b3f33 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 337f469d1a 
  ql/src/test/org/apache/hadoop/hive/ql/io/TestAcidUtils.java f351f04b08 
  ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java 
e4440e9136 
  ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcRawRecordMerger.java 
f63c40a7b5 
  streaming/src/test/org/apache/hive/streaming/TestStreaming.java 3a3b267927 


Diff: https://reviews.apache.org/r/72532/diff/3/

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


Testing
---


Thanks,

Peter Varga

Re: Review Request 72528: ValidTxnManager doesn't consider txns opened and committed between snapshot generation and locking when evaluating ValidTxnListState

[Peter Varga via Review Board]

> On May 20, 2020, 3:16 p.m., Peter Varga wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 686 (patched)
> > 
> >
> > I have concerns here, but I am not sure if they are well founded or 
> > not. I think this will break what the outside world thinks of snapshot 
> > isolation. I might have a hypothetical client that inserts lots of data in 
> > a source table and sometimes issue a merge statement from the source to the 
> > target table. They have some requirement that the target table can not have 
> > partial data regarding some property. Example they inserting sales data, 
> > and the target table can not contain half the data of a day, it can either 
> > have all or none. So what the clients does, it will issue the inserts into 
> > the source table synchronously ordered by the date and when it gets to a 
> > next day it issue a merge statement asynchronously and continues to inserts 
> > the data for the next day synchronously. And it might think that it is save 
> > to do so, since the merge statement has a snapshot it will not see the data 
> > inserted afterwards. But with this change it will break.
> > It might not be the best example, since how would the client know when 
> > the snapshot is actually captured. But I am not familiar enough with the 
> > ecosystem, does anything use the Hive by issuing the compile and run 
> > separately? Because there you could be sure before this change, that the 
> > compilation order also meant snapshot order. So summarized, I don't know 
> > what the outside world excepts of the snapshot isolation.
> 
> Denys Kuzmenko wrote:
> insert into source and merge from source into target won't conflict with 
> each other, they touch different tables. Maybe I missing something here...
> 
> Peter Varga wrote:
> My example was not perfect. I don't mean that it will conflict with the 
> insert into the source table. It can conflict with some other client's 
> transaction. My main point is, after the conflict is noticed and you 
> regenerate the snapshot it will starts to read results from transactions that 
> were opened and committed after the original query was compiled, and I'm just 
> trying to figure out, what kinf of problems can it cause, if any. In my 
> example you start to read records inserted later, but what if somebody added 
> a new partition since the compilation, wouldn't it cause problem?
> 
> Denys Kuzmenko wrote:
> probably there might be an issue as we won't create any locks for the 
> newly created partition, however we'll start reading it.
> instead of rollback & retry on Hive side we might consider to just fail 
> and let the user re-try.
> 
> Denys Kuzmenko wrote:
> however it still leaves the question what happens now in Hive when 
> somebody adds a new partition (insert with dynamic partitioning) since the 
> compilation (merge insert). I'll test this out.

You could construct an example like this:
1. open and compile transaction 1 that merge inserts data from a partitioned 
source table that has a few partition.
2. Open, run and commit transaction 2 that inserts data to an old and a new 
partition to the source table.
3. Open, run and commit transaction 3 that inserts data to the target table of 
the merge statement, that will retrigger a snapshot generation in transaction 1.
4. Run transaction 1, the snapshot will be regenerated, and I think it will 
read partial data from transaction 2 breaking the ACID properties.

But we should try the test without your patch with a little different setup.
Switch the transaction order:
1. compile transaction 1 that inserts data to an old and a new partition of the 
source table.
2. compile transaction 2 that insert data to the target table
2. compile transaction 3 that merge inserts data from the source table to the 
target table
3. run and commit transaction 1
4. run and commit transaction 2
5. run transaction 3, since it cointains 1 and 2 in its snaphot the 
isValidTxnListState will be triggered without your patch and I think it 
possible that we do a partial read of the transaction 1 for the same reasons.


- Peter


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


On May 19, 2020, 11:19 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72528/
> ---
> 
> (Updated May 19, 2020, 11:19 a.m.)
> 
> 
> Review request for hive, Jesús Camacho Rodríguez, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23503
> https://issues.apache.org/jira/browse/HIVE-23503
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 

Re: Review Request 72488: HIVE-23413: New config to skip all locks

[Peter Varga via Review Board]

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

(Updated June 3, 2020, 9:20 a.m.)


Review request for hive, Denys Kuzmenko and Peter Vary.


Changes
---

Renamed the config


Repository: hive-git


Description
---

>From time-to-time some query is blocked on locks which should not.

To have a quick workaround for this we should have a config which the user can 
set in the session to disable acquiring/checking locks, so we can provide it 
immediately and then later investigate and fix the root cause.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java abd12c9a82 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 71afcbdc68 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
8a15b7cc5d 


Diff: https://reviews.apache.org/r/72488/diff/2/

Changes: https://reviews.apache.org/r/72488/diff/1-2/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72488: HIVE-23413: New config to skip all locks

[Peter Varga via Review Board]

> On June 3, 2020, 6:53 a.m., Denys Kuzmenko wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
> > Lines 2737 (patched)
> > 
> >
> > I would name config property - HIVE_TXN_DISABLE_LOCKS, to give more 
> > clarification on  its purpose.

Thanks for the review, renamed the config.


- Peter


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


On June 3, 2020, 9:20 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72488/
> ---
> 
> (Updated June 3, 2020, 9:20 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> From time-to-time some query is blocked on locks which should not.
> 
> To have a quick workaround for this we should have a config which the user 
> can set in the session to disable acquiring/checking locks, so we can provide 
> it immediately and then later investigate and fix the root cause.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java abd12c9a82 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 71afcbdc68 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 8a15b7cc5d 
> 
> 
> Diff: https://reviews.apache.org/r/72488/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72532: HIVE-23495 AcidUtils.getAcidState cleanup

[Peter Varga via Review Board]

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

(Updated May 21, 2020, 1:23 p.m.)


Review request for hive, Karen Coppage and Peter Vary.


Repository: hive-git


Description
---

since HIVE-21225 there are two redundant implementation of the 
AcidUtils.getAcidState.

The previous implementation (without the recursive listing) can be removed.

Also the performance can be improved, by removing unnecessary fileStatus calls.


Diffs (updated)
-

  
hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/TestStreaming.java
 569de706df 
  
hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/mutate/StreamingAssert.java
 86f762e97c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java bf332bc0b8 
  ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java ca234cfb37 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 1059cb227f 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcRawRecordMerger.java 
16c915959c 
  
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java
 598220b0c4 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java 5fa3d9ad42 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
018c73376f 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java fa2ede3738 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MinorQueryCompactor.java 
d83a50f555 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMajorQueryCompactor.java 
5e11d8d2d8 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMinorQueryCompactor.java 
1bdec7df2d 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java a96cf1e731 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 366282a30f 
  ql/src/test/org/apache/hadoop/hive/ql/io/TestAcidUtils.java 9e6d47ebc5 
  ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java 
12a15a16eb 
  ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcRawRecordMerger.java 
f63c40a7b5 
  streaming/src/test/org/apache/hive/streaming/TestStreaming.java 6101caac66 


Diff: https://reviews.apache.org/r/72532/diff/2/

Changes: https://reviews.apache.org/r/72532/diff/1-2/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72528: ValidTxnManager doesn't consider txns opened and committed between snapshot generation and locking when evaluating ValidTxnListState

[Peter Varga via Review Board]

> On May 20, 2020, 3:16 p.m., Peter Varga wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java
> > Lines 686 (patched)
> > 
> >
> > I have concerns here, but I am not sure if they are well founded or 
> > not. I think this will break what the outside world thinks of snapshot 
> > isolation. I might have a hypothetical client that inserts lots of data in 
> > a source table and sometimes issue a merge statement from the source to the 
> > target table. They have some requirement that the target table can not have 
> > partial data regarding some property. Example they inserting sales data, 
> > and the target table can not contain half the data of a day, it can either 
> > have all or none. So what the clients does, it will issue the inserts into 
> > the source table synchronously ordered by the date and when it gets to a 
> > next day it issue a merge statement asynchronously and continues to inserts 
> > the data for the next day synchronously. And it might think that it is save 
> > to do so, since the merge statement has a snapshot it will not see the data 
> > inserted afterwards. But with this change it will break.
> > It might not be the best example, since how would the client know when 
> > the snapshot is actually captured. But I am not familiar enough with the 
> > ecosystem, does anything use the Hive by issuing the compile and run 
> > separately? Because there you could be sure before this change, that the 
> > compilation order also meant snapshot order. So summarized, I don't know 
> > what the outside world excepts of the snapshot isolation.
> 
> Denys Kuzmenko wrote:
> insert into source and merge from source into target won't conflict with 
> each other, they touch different tables. Maybe I missing something here...

My example was not perfect. I don't mean that it will conflict with the insert 
into the source table. It can conflict with some other client's transaction. My 
main point is, after the conflict is noticed and you regenerate the snapshot it 
will starts to read results from transactions that were opened and committed 
after the original query was compiled, and I'm just trying to figure out, what 
kinf of problems can it cause, if any. In my example you start to read records 
inserted later, but what if somebody added a new partition since the 
compilation, wouldn't it cause problem?


- Peter


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


On May 19, 2020, 11:19 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72528/
> ---
> 
> (Updated May 19, 2020, 11:19 a.m.)
> 
> 
> Review request for hive, Jesús Camacho Rodríguez, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23503
> https://issues.apache.org/jira/browse/HIVE-23503
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> ValidTxnManager doesn't consider txns opened and committed between snapshot 
> generation and locking when evaluating ValidTxnListState. This cause issues 
> like duplicate insert in case of concurrent merge insert & insert.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java e70c92eef4 
>   ql/src/java/org/apache/hadoop/hive/ql/DriverContext.java a8c83fc504 
>   ql/src/java/org/apache/hadoop/hive/ql/ValidTxnManager.java 7d49c57dda 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 71afcbdc68 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> 0383881acc 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 600289f837 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 8a15b7cc5d 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  65df9c2ba9 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  887d4303f4 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java
>  312936efa8 
>   storage-api/src/java/org/apache/hadoop/hive/common/ValidReadTxnList.java 
> b8ff03f9c4 
>   storage-api/src/java/org/apache/hadoop/hive/common/ValidTxnList.java 
> d4c3b09730 
> 
> 
> Diff: https://reviews.apache.org/r/72528/diff/1/
> 
> 
> Testing
> ---
> 
> DbTxnManager tests.
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Review Request 72532: HIVE-23495 AcidUtils.getAcidState cleanup

[Peter Varga via Review Board]

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

Review request for hive, Karen Coppage and Peter Vary.


Repository: hive-git


Description
---

since HIVE-21225 there are two redundant implementation of the 
AcidUtils.getAcidState.

The previous implementation (without the recursive listing) can be removed.

Also the performance can be improved, by removing unnecessary fileStatus calls.


Diffs
-

  
hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/TestStreaming.java
 569de706df 
  
hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/mutate/StreamingAssert.java
 86f762e97c 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 270c5909fc 
  ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java ca234cfb37 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 1059cb227f 
  ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcRawRecordMerger.java 
16c915959c 
  
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java
 598220b0c4 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java 5fa3d9ad42 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java 
05ea38c5af 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java fa2ede3738 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MinorQueryCompactor.java 
4d0e5f703e 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMajorQueryCompactor.java 
724a4375b7 
  
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MmMinorQueryCompactor.java 
1cd95f8015 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java a96cf1e731 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 366282a30f 
  ql/src/test/org/apache/hadoop/hive/ql/io/TestAcidUtils.java 9e6d47ebc5 
  ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java 
12a15a16eb 
  ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcRawRecordMerger.java 
f63c40a7b5 
  streaming/src/test/org/apache/hive/streaming/TestStreaming.java 6101caac66 


Diff: https://reviews.apache.org/r/72532/diff/1/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72528: ValidTxnManager doesn't consider txns opened and committed between snapshot generation and locking when evaluating ValidTxnListState

[Peter Varga via Review Board]

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



The patch looks great, but I have concers. See below.


ql/src/java/org/apache/hadoop/hive/ql/Driver.java
Lines 686 (patched)


I have concerns here, but I am not sure if they are well founded or not. I 
think this will break what the outside world thinks of snapshot isolation. I 
might have a hypothetical client that inserts lots of data in a source table 
and sometimes issue a merge statement from the source to the target table. They 
have some requirement that the target table can not have partial data regarding 
some property. Example they inserting sales data, and the target table can not 
contain half the data of a day, it can either have all or none. So what the 
clients does, it will issue the inserts into the source table synchronously 
ordered by the date and when it gets to a next day it issue a merge statement 
asynchronously and continues to inserts the data for the next day 
synchronously. And it might think that it is save to do so, since the merge 
statement has a snapshot it will not see the data inserted afterwards. But with 
this change it will break.
It might not be the best example, since how would the client know when the 
snapshot is actually captured. But I am not familiar enough with the ecosystem, 
does anything use the Hive by issuing the compile and run separately? Because 
there you could be sure before this change, that the compilation order also 
meant snapshot order. So summarized, I don't know what the outside world 
excepts of the snapshot isolation.


- Peter Varga


On May 19, 2020, 11:19 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72528/
> ---
> 
> (Updated May 19, 2020, 11:19 a.m.)
> 
> 
> Review request for hive, Jesús Camacho Rodríguez, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23503
> https://issues.apache.org/jira/browse/HIVE-23503
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> ValidTxnManager doesn't consider txns opened and committed between snapshot 
> generation and locking when evaluating ValidTxnListState. This cause issues 
> like duplicate insert in case of concurrent merge insert & insert.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java e70c92eef4 
>   ql/src/java/org/apache/hadoop/hive/ql/DriverContext.java a8c83fc504 
>   ql/src/java/org/apache/hadoop/hive/ql/ValidTxnManager.java 7d49c57dda 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 71afcbdc68 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java 
> 0383881acc 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 
> 600289f837 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 8a15b7cc5d 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
>  65df9c2ba9 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
>  887d4303f4 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java
>  312936efa8 
>   storage-api/src/java/org/apache/hadoop/hive/common/ValidReadTxnList.java 
> b8ff03f9c4 
>   storage-api/src/java/org/apache/hadoop/hive/common/ValidTxnList.java 
> d4c3b09730 
> 
> 
> Diff: https://reviews.apache.org/r/72528/diff/1/
> 
> 
> Testing
> ---
> 
> DbTxnManager tests.
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72480: HIVE-23242 Fix flaky tests testHouseKeepingThreadExistence

[Peter Varga via Review Board]

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

(Updated May 20, 2020, 2:03 p.m.)


Review request for hive, Miklos Gergely and Peter Vary.


Repository: hive-git


Description
---

Fix the timing to avoid flakyness.


Diffs (updated)
-

  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreHousekeepingLeaderTestBase.java
 a39a9c8e04 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreTaskThreadAlwaysTestImpl.java
 4cd2c58896 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl1.java
 c590b6aad5 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl2.java
 5b50f66c51 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeader.java
 03a8161ea4 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeaderEmptyConfig.java
 75ea637503 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingNonLeader.java
 0341d3c03b 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 57c006b872 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/MetaStoreTestUtils.java
 0e2c35acaa 


Diff: https://reviews.apache.org/r/72480/diff/3/

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


Testing
---


Thanks,

Peter Varga

Re: Review Request 72480: HIVE-23242 Fix flaky tests testHouseKeepingThreadExistence

[Peter Varga via Review Board]

> On May 19, 2020, 7:54 a.m., Peter Vary wrote:
> >

Thank you for the review, fixed the issues + removed some parameters from the 
HMS.startMetaStore, since they were just implementation details unneccessary 
exposed.


- Peter


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


On May 20, 2020, 2:03 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72480/
> ---
> 
> (Updated May 20, 2020, 2:03 p.m.)
> 
> 
> Review request for hive, Miklos Gergely and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Fix the timing to avoid flakyness.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreHousekeepingLeaderTestBase.java
>  a39a9c8e04 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreTaskThreadAlwaysTestImpl.java
>  4cd2c58896 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl1.java
>  c590b6aad5 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl2.java
>  5b50f66c51 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeader.java
>  03a8161ea4 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeaderEmptyConfig.java
>  75ea637503 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingNonLeader.java
>  0341d3c03b 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  57c006b872 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/MetaStoreTestUtils.java
>  0e2c35acaa 
> 
> 
> Diff: https://reviews.apache.org/r/72480/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

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

(Updated May 20, 2020, 12:24 p.m.)


Review request for hive and Denys Kuzmenko.


Changes
---

Removed the static imports.


Repository: hive-git


Description
---

* Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
  * Remove TxnStatus character constants and use the enum values


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 d2efc595a5 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxn.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxnList.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OperationType.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 89ddccbbda 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStatus.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72465/diff/4/

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


Testing
---


Thanks,

Peter Varga

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

> On May 18, 2020, 7:33 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStatus.java
> > Lines 32 (patched)
> > 
> >
> > Could you please refactor here as well LockTypeUtil to LockType enum.
> 
> Denys Kuzmenko wrote:
> move to txn package + overwrite toString(), this would allow to get rid 
> of static exclWrite, sharedWrite, sharedRead methods

The LockType enum is part of the thrift interface. I could duplicate it, but I 
am not sure it will be cleaner that way. I don't know why they went with this 
weird encoding, that the enums are represented as ints in the interface and as 
char-s in the db. So many unnecessary switch between the two representation.


- Peter


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


On May 7, 2020, 11:58 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72465/
> ---
> 
> (Updated May 7, 2020, 11:58 a.m.)
> 
> 
> Review request for hive and Denys Kuzmenko.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
>   * Remove TxnStatus character constants and use the enum values
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  d59f863b11 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxn.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxnList.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OperationType.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  7c3937520c 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStatus.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72465/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Review Request 72488: HIVE-23413: New config to skip all locks

[Peter Varga via Review Board]

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

Review request for hive and Peter Vary.


Repository: hive-git


Description
---

>From time-to-time some query is blocked on locks which should not.

To have a quick workaround for this we should have a config which the user can 
set in the session to disable acquiring/checking locks, so we can provide it 
immediately and then later investigate and fix the root cause.


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 60ae06a49a 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
2adabe7058 


Diff: https://reviews.apache.org/r/72488/diff/1/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72480: HIVE-23242 Fix flaky tests testHouseKeepingThreadExistence

[Peter Varga via Review Board]

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

(Updated May 8, 2020, 9:46 a.m.)


Review request for hive, Miklos Gergely and Peter Vary.


Repository: hive-git


Description
---

Fix the timing to avoid flakyness.


Diffs (updated)
-

  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreHousekeepingLeaderTestBase.java
 a39a9c8e04 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreTaskThreadAlwaysTestImpl.java
 4cd2c58896 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl1.java
 c590b6aad5 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl2.java
 5b50f66c51 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeader.java
 03a8161ea4 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeaderEmptyConfig.java
 75ea637503 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingNonLeader.java
 0341d3c03b 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 7bba8d6ee6 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/MetaStoreTestUtils.java
 2702e69f86 


Diff: https://reviews.apache.org/r/72480/diff/2/

Changes: https://reviews.apache.org/r/72480/diff/1-2/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

> On May 4, 2020, 3 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 107 (original), 110 (patched)
> > 
> >
> > Could we add helper methods into the enum to avoid calling quoteChar 
> > everywhere, like TxnSatus.aborted()?

99% of the places we use the quoted strings so I changed to toString, this will 
be the most readable way I think.


> On May 4, 2020, 3 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
> > Lines 400 (patched)
> > 
> >
> > Is is a generic txn object (i.e TxnInfo)? I don't think TxnUtils is a 
> > good place for domain objects.
> 
> Peter Varga wrote:
> I didn't really know which way to go. The TxnInfo is missing the type 
> field and I did not want to change the api.
> The tables used by txnhandler don't have domain objects, should I create 
> one in the model package for txns? I think that would be confusing too, since 
> it would seem like the txns table is managed by jdo.
> What do you suggest?
> 
> Denys Kuzmenko wrote:
> I would create OpenTxnList under org.apache.hadoop.hive.metastore.txn 
> (similarily to CompactionInfo) - same as you did with extra 2 methods:
> - toOpenTxns (OpenTxnsResponse)
> - toOpenTxnsInfo (OpenTxnsInfoResponse)
> 
> and place dto conversion logic there

Moved the OpenTxn, OpenTxnList and the TxnState and OperationTypee enums to the 
txn package.


- Peter


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


On May 7, 2020, 11:58 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72465/
> ---
> 
> (Updated May 7, 2020, 11:58 a.m.)
> 
> 
> Review request for hive and Denys Kuzmenko.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
>   * Remove TxnStatus character constants and use the enum values
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  d59f863b11 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxn.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxnList.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OperationType.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  7c3937520c 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStatus.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72465/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

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

(Updated May 7, 2020, 11:58 a.m.)


Review request for hive and Denys Kuzmenko.


Repository: hive-git


Description
---

* Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
  * Remove TxnStatus character constants and use the enum values


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 d59f863b11 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxn.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OpenTxnList.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/OperationType.java
 PRE-CREATION 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 7c3937520c 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStatus.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72465/diff/3/

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


Testing
---


Thanks,

Peter Varga

Review Request 72480: HIVE-23242 Fix flaky tests testHouseKeepingThreadExistence

[Peter Varga via Review Board]

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

Review request for hive, Miklos Gergely and Peter Vary.


Repository: hive-git


Description
---

Fix the timing to avoid flakyness.


Diffs
-

  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreHousekeepingLeaderTestBase.java
 a39a9c8e04 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreTaskThreadAlwaysTestImpl.java
 4cd2c58896 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl1.java
 c590b6aad5 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/RemoteMetastoreTaskThreadTestImpl2.java
 5b50f66c51 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeader.java
 03a8161ea4 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeaderEmptyConfig.java
 75ea637503 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingNonLeader.java
 0341d3c03b 


Diff: https://reviews.apache.org/r/72480/diff/1/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

[Peter Varga via Review Board]

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


Ship it!




Ship It!

- Peter Varga


On May 5, 2020, 8:33 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 5, 2020, 8:33 a.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/3/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

> On May 4, 2020, 3 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
> > Lines 400 (patched)
> > 
> >
> > Is is a generic txn object (i.e TxnInfo)? I don't think TxnUtils is a 
> > good place for domain objects.

I didn't really know which way to go. The TxnInfo is missing the type field and 
I did not want to change the api.
The tables used by txnhandler don't have domain objects, should I create one in 
the model package for txns? I think that would be confusing too, since it would 
seem like the txns table is managed by jdo.
What do you suggest?


- Peter


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


On May 4, 2020, 1:22 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72465/
> ---
> 
> (Updated May 4, 2020, 1:22 p.m.)
> 
> 
> Review request for hive and Denys Kuzmenko.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
>   * Remove TxnStatus character constants and use the enum values
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  a1bc10955a 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  8fded608d0 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
>  4ee1a45aae 
> 
> 
> Diff: https://reviews.apache.org/r/72465/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

[Peter Varga via Review Board]

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



Looks good, just one question.


ql/src/java/org/apache/hadoop/hive/ql/Context.java
Lines 1198 (patched)


Do you need a separate field for that, you could just check the operation 
type?


- Peter Varga


On May 4, 2020, 4:33 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 4, 2020, 4:33 p.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/2/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

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

(Updated May 4, 2020, 1:22 p.m.)


Review request for hive and Denys Kuzmenko.


Changes
---

small performance improvement


Repository: hive-git


Description
---

* Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
  * Remove TxnStatus character constants and use the enum values


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 a1bc10955a 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 8fded608d0 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
 4ee1a45aae 


Diff: https://reviews.apache.org/r/72465/diff/2/

Changes: https://reviews.apache.org/r/72465/diff/1-2/


Testing
---


Thanks,

Peter Varga

Review Request 72465: HIVE-23340 TxnHandler cleanup

[Peter Varga via Review Board]

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

Review request for hive and Denys Kuzmenko.


Repository: hive-git


Description
---

* Merge getOpenTxns and getOpenTxnInfo to avoid code duplication
  * Remove TxnStatus character constants and use the enum values


Diffs
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 a1bc10955a 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 8fded608d0 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
 4ee1a45aae 


Diff: https://reviews.apache.org/r/72465/diff/1/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

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

(Updated April 28, 2020, 3:38 p.m.)


Review request for hive and Peter Vary.


Repository: hive-git


Description
---

* Use sequences for TXN_ID generation.
  * Make it possible to open transactions in parallel
  * drop Oracle 11g support


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java b4dac4346e 
  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 37a5862791 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
7c8903fbae 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
3916e88a9d 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
6525ffc00a 
  ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 1435269ed3 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/ITestDbTxnManager.java 
a085e9ff6f 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
f90396b2a3 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
 PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
1151466f8c 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 a874121e12 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
 49b737ecf9 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 2344c2d5f6 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 97a083399a 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 fe39b0b36e 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
 87130a519d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 366b6f02c1 
  
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
 8a3cd56658 
  
standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
 2e0177723d 
  
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
 9f3951575b 
  
standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
 0512a45cad 
  
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
 4b82e36ab4 
  
standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
 db398e5f66 
  
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
 1be83fc4a9 
  
standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
 e6e30160af 
  
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
 b90cecb173 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
 c1a1629548 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
 a6d22d19ef 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
 0b070e19ac 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72388/diff/6/

Changes: https://reviews.apache.org/r/72388/diff/5-6/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 27, 2020, 3:04 p.m., Peter Vary wrote:
> > Fix it and ship it

Thank you for the reviews, fixed the issues and got a green run.


> On April 27, 2020, 3:04 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
> > Lines 76 (patched)
> > 
> >
> > TXN_ID should be 10-1

I left the max committed insert and inserted the 10 value this way if 
some administrator want to fix the db manually after the schematool run, it 
will be easier.


> On April 27, 2020, 3:04 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
> > Line 27 (original), 27 (patched)
> > 
> >
> > Is this public?

Yes it is.


- Peter


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


On April 24, 2020, 3:40 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 24, 2020, 3:40 p.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Use sequences for TXN_ID generation.
>   * Make it possible to open transactions in parallel
>   * drop Oracle 11g support
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 37a5862791 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> f512c1df19 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
> 6525ffc00a 
>   ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 
> 1435269ed3 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 497cedd61f 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
> 1151466f8c 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  a874121e12 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
>  49b737ecf9 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  2344c2d5f6 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  385f9d72cd 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  4a6fa6f620 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
>  87130a519d 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
>  8a3cd56658 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
>  2e0177723d 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
>  9f3951575b 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
>  0512a45cad 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
>  4b82e36ab4 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
>  db398e5f66 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
>  1be83fc4a9 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
>  e6e30160af 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
>  b90cecb173 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
>  c1a1629548 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
>  a6d22d19ef 
>   
> 

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 24, 2020, 3:14 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 1453 (patched)
> > 
> >
> > Why is this change?

Just to satisfy the yetus checkstyle, it does not like the else statement 
without an op :)


> On April 24, 2020, 3:14 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
> > Line 47 (original), 47 (patched)
> > 
> >
> > What is the reason behind this change?

an interface variable will be public static final anyway.


- Peter


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


On April 24, 2020, 3:40 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 24, 2020, 3:40 p.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Use sequences for TXN_ID generation.
>   * Make it possible to open transactions in parallel
>   * drop Oracle 11g support
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 37a5862791 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> f512c1df19 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
> 6525ffc00a 
>   ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 
> 1435269ed3 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 497cedd61f 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
> 1151466f8c 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  a874121e12 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
>  49b737ecf9 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  2344c2d5f6 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  385f9d72cd 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  4a6fa6f620 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
>  87130a519d 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
>  8a3cd56658 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
>  2e0177723d 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
>  9f3951575b 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
>  0512a45cad 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
>  4b82e36ab4 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
>  db398e5f66 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
>  1be83fc4a9 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
>  e6e30160af 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
>  b90cecb173 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
>  c1a1629548 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
>  a6d22d19ef 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
>  0b070e19ac 
>   
> 

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
> > Lines 1068 (patched)
> > 
> >
> > Is it important? I thought TXNS table is cleaned before test.

you are right, the clean is not neccessary, just to wait for the end of the 
opentimeperiod, fixed.


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 578 (original), 588 (patched)
> > 
> >
> > Shouldn't it be under debug?

fixed


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
> > Lines 86 (patched)
> > 
> >
> > I think, in future, we should move out from this class everyting test 
> > related.

Actually, if you look at the javadoc at the class, this class supposed to be 
used only by tests :) But I agree that we shouuld separate the test and the 
production code.


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 228 (patched)
> > 
> >
> > Could we exend TxnStatus enum with char representation?

I have seen the TODO, will do that in a follow up Jira.


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 433 (original), 517 (patched)
> > 
> >
> > Should we log here something meaningfull under error level?

Actually the checkRetryable will log every detail neccessary, I don't know why 
the commit and rollback is logged everywhere, but it is quit consistent in this 
class.


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 559 (patched)
> > 
> >
> > Could we extract openTxnLowBoundary calc logic? it's the same as in 
> > getOPenTxnInfo

We talked about this with Peter, I would like to extact the whole logic from 
getOpenTxns and getOpenTXnInfo to remove the duplication, I just need to create 
some inner datastructure, because now both the return values missing somethings 
from the other. I will do that in a next jira.


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 575 (original), 681 (patched)
> > 
> >
> > Not very useful log trace

do we want change this everywhere? I would prefer to be consistent in this 
class.


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 696 (patched)
> > 
> >
> > Should we log this under error, otherwise we could miss some issues in 
> > a system?

fixed


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 815 (patched)
> > 
> >
> > Based on method declaration I wouldn't expect it to return something - 
> > list of txnIds, but not mutate state using reference.

fixed


> On April 24, 2020, 2:16 p.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 891 (patched)
> > 
> >
> > redundant " + "

fixed


- Peter


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


On April 24, 2020, 3:40 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 24, 2020, 3:40 p.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> 

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

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

(Updated April 24, 2020, 3:40 p.m.)


Review request for hive and Peter Vary.


Repository: hive-git


Description
---

* Use sequences for TXN_ID generation.
  * Make it possible to open transactions in parallel
  * drop Oracle 11g support


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 37a5862791 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
f512c1df19 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
6525ffc00a 
  ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 1435269ed3 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
497cedd61f 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
 PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
1151466f8c 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 a874121e12 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
 49b737ecf9 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 2344c2d5f6 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 385f9d72cd 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 4a6fa6f620 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
 87130a519d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
 8a3cd56658 
  
standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
 2e0177723d 
  
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
 9f3951575b 
  
standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
 0512a45cad 
  
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
 4b82e36ab4 
  
standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
 db398e5f66 
  
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
 1be83fc4a9 
  
standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
 e6e30160af 
  
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
 b90cecb173 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
 c1a1629548 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
 a6d22d19ef 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
 0b070e19ac 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72388/diff/5/

Changes: https://reviews.apache.org/r/72388/diff/4-5/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

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

(Updated April 23, 2020, 2:48 p.m.)


Review request for hive and Peter Vary.


Changes
---

Modified the txn lock to use the same connection as the opentxns statements for 
performance gains, for that I had to give up to use table lock on every db, but 
actually the code got simpler.


Repository: hive-git


Description
---

* Use sequences for TXN_ID generation.
  * Make it possible to open transactions in parallel
  * drop Oracle 11g support


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 37a5862791 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
1d211857bf 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
6525ffc00a 
  ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 1435269ed3 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
73d3b91585 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
 PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
1151466f8c 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 a874121e12 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
 49b737ecf9 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 2344c2d5f6 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 385f9d72cd 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 d0d0320584 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
 87130a519d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
 8a3cd56658 
  
standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
 2e0177723d 
  
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
 9f3951575b 
  
standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
 0512a45cad 
  
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
 4b82e36ab4 
  
standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
 db398e5f66 
  
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
 1be83fc4a9 
  
standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
 e6e30160af 
  
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
 b90cecb173 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
 c1a1629548 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
 a6d22d19ef 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
 0b070e19ac 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72388/diff/4/

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


Testing
---


Thanks,

Peter Varga

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 902 (patched)
> > 
> >
> > Can it be null? Could we add constraint on db level instead of multiple 
> > checks in backend code?
> 
> Peter Varga wrote:
> It should not be null, but the constraint is that there must be always 
> one record in the table, not a nonnull column.
> 
> Denys Kuzmenko wrote:
> ok, don't you cover this with if (!maxTxnIdRs.next()) { .. } ?
> 
> Peter Varga wrote:
> No, if there is no record, the max will return one row with a null value
> 
> Denys Kuzmenko wrote:
> ok, got it, in this case why are we doing throw new 
> IllegalStateException("Scalar query returned no rows?!?!!")?

removed that.


- Peter


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


On April 21, 2020, 12:51 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 21, 2020, 12:51 p.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Use sequences for TXN_ID generation.
>   * Make it possible to open transactions in parallel
>   * drop Oracle 11g support
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 37a5862791 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> 1d211857bf 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
> 6525ffc00a 
>   ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 
> 1435269ed3 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 73d3b91585 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
> 1151466f8c 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  a874121e12 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
>  49b737ecf9 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  2344c2d5f6 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  bb29410e7d 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  e7910c1c5d 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
>  87130a519d 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
>  8a3cd56658 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
>  2e0177723d 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
>  9f3951575b 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
>  0512a45cad 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
>  4b82e36ab4 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
>  db398e5f66 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
>  1be83fc4a9 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
>  e6e30160af 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
>  b90cecb173 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
>  c1a1629548 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
>  a6d22d19ef 
>   
> 

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 699 (patched)
> > 
> >
> > openTxns(dbConn, stmt, rqst) is used in replTableWriteIdState as well. 
> > Do we have to check there for timeout as well?
> 
> Peter Varga wrote:
> This is an alarming catch. What kind of queries run on a replication. I 
> put there the table lock, but it will be a rather long synchronized block. On 
> the other hand aren't the replication tasks somehow serialized? If there are 
> no select queries on replication the timeout is not needed, if there are, we 
> might have a problem, because we do a lot of think before the commit, where 
> we can measure.
> And the most seriuos problem, if I see correctly, the replication will 
> use this newly opened transactionId-s to write into the TXN_TO_WRITE_ID, 
> wouldn't be a problem if this will be different than the original value?

As we discussed, the txn_id change is not a problem, and we don't need either 
the timeout check or the txnLock, because all of the opened transactions will 
be aborted in the same rdbms transaction


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 780 (patched)
> > 
> >
> > TXN_META_INFO? What is it used for before? Or is it new? Could we use a 
> > specific "state" for example?
> 
> Denys Kuzmenko wrote:
> My understanding was, it's kinda marker to flag the new txn entries and 
> get their ids.
> 
> Peter Varga wrote:
> It is a marker, I just used an existing field. It is updated to null 
> later.

change it to txn_state


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 641 (original), 783 (patched)
> > 
> >
> > Could we use a correct PreparedStatement, with the values set without 
> > the "hacky" quoting?
> > 
> > Like: pstmt.setXX for state/user/host/type/metainfo?
> > 
> > Or does this have a noticable performance gain?
> 
> Peter Varga wrote:
> Changed the metainfo select back and update to preparedstatements. The 
> original insert will entirely change in your batch insert patch if I know 
> correctly, so I will leave that.

incorporated your batchinsert patch also


- Peter


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


On April 21, 2020, 12:51 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 21, 2020, 12:51 p.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Use sequences for TXN_ID generation.
>   * Make it possible to open transactions in parallel
>   * drop Oracle 11g support
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 37a5862791 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> 1d211857bf 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
> 6525ffc00a 
>   ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 
> 1435269ed3 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 73d3b91585 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
> 1151466f8c 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  a874121e12 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
>  49b737ecf9 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  2344c2d5f6 
>   
> 

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

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

(Updated April 21, 2020, 12:51 p.m.)


Review request for hive and Peter Vary.


Repository: hive-git


Description
---

* Use sequences for TXN_ID generation.
  * Make it possible to open transactions in parallel
  * drop Oracle 11g support


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 37a5862791 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
1d211857bf 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
6525ffc00a 
  ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 1435269ed3 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
73d3b91585 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
 PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
1151466f8c 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 a874121e12 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
 49b737ecf9 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 2344c2d5f6 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 bb29410e7d 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 e7910c1c5d 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
 87130a519d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
 8a3cd56658 
  
standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
 2e0177723d 
  
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
 9f3951575b 
  
standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
 0512a45cad 
  
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
 4b82e36ab4 
  
standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
 db398e5f66 
  
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
 1be83fc4a9 
  
standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
 e6e30160af 
  
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
 b90cecb173 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
 c1a1629548 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
 a6d22d19ef 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
 0b070e19ac 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72388/diff/3/

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


Testing
---


Thanks,

Peter Varga

Re: Review Request 72380: HIVE-23207 Create integration tests for TxnManager for different rdbms metastores

[Peter Varga via Review Board]

> On April 17, 2020, 7:03 p.m., Peter Vary wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestMetaStoreHandler.java
> > Lines 67 (patched)
> > 
> >
> > Why is this needed?

These remained here because of some failing tests, but I was able to fix all of 
those, so I removed it.


- Peter


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


On April 21, 2020, 10:22 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72380/
> ---
> 
> (Updated April 21, 2020, 10:22 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Zoltan Chovan.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> In the final version the prepDb creates the transactional tables with the 
> init schema (and ignores the others if they exists). The cleanDb resets the 
> database to the starting point. So between the test cases the cleanDb call is 
> enough. If the prepDb is called unneccessary it will just check if the txns 
> table exist and then return, so it will be fast
> 
> 
> Diffs
> -
> 
>   contrib/src/test/queries/clientpositive/url_hook.q 512e579fb8 
>   itests/hive-blobstore/pom.xml 09955c55f3 
>   itests/qtest-accumulo/pom.xml a35d2a8a10 
>   itests/qtest-druid/pom.xml cc0cceff68 
>   itests/qtest-kudu/pom.xml f23399fa37 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliAdapter.java 
> fcfc79059a 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java
>  5b08f8b894 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestMetaStoreHandler.java
>  b86d736a89 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 
> 38e4bac2cc 
>   pom.xml b29c06c69e 
>   ql/pom.xml a0e77a1d32 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> 1d211857bf 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandlerNoConnectionPool.java
>  ebe4880e3a 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/ITestDbTxnManager.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 73d3b91585 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java
>  3e56ad513c 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  bb29410e7d 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  e7910c1c5d 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
>  5f3db52c2f 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Mysql.java
>  c537d95470 
> 
> 
> Diff: https://reviews.apache.org/r/72380/diff/2/
> 
> 
> Testing
> ---
> 
> On my machine the 50 tests in TestDbTxnManager2 on postgres runs under 5 
> minutes.
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 902 (patched)
> > 
> >
> > Can it be null? Could we add constraint on db level instead of multiple 
> > checks in backend code?
> 
> Peter Varga wrote:
> It should not be null, but the constraint is that there must be always 
> one record in the table, not a nonnull column.
> 
> Denys Kuzmenko wrote:
> ok, don't you cover this with if (!maxTxnIdRs.next()) { .. } ?

No, if there is no record, the max will return one row with a null value


- Peter


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


On April 21, 2020, 7:53 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 21, 2020, 7:53 a.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Use sequences for TXN_ID generation.
>   * Make it possible to open transactions in parallel
>   * drop Oracle 11g support
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 37a5862791 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> 1d211857bf 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
> 6525ffc00a 
>   ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 
> 1435269ed3 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 73d3b91585 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
>  PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
> 1151466f8c 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  a874121e12 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
>  49b737ecf9 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  2344c2d5f6 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  bb29410e7d 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  e7910c1c5d 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
>  87130a519d 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
>  8a3cd56658 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
>  2e0177723d 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
>  9f3951575b 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
>  0512a45cad 
>   
> standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
>  4b82e36ab4 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
>  db398e5f66 
>   
> standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
>  1be83fc4a9 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
>  e6e30160af 
>   
> standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
>  b90cecb173 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
>  c1a1629548 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
>  a6d22d19ef 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
>  0b070e19ac 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
>  PRE-CREATION 
> 
> 
> Diff: 

Re: Review Request 72380: HIVE-23207 Create integration tests for TxnManager for different rdbms metastores

[Peter Varga via Review Board]

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

(Updated April 21, 2020, 10:22 a.m.)


Review request for hive, Denys Kuzmenko and Zoltan Chovan.


Repository: hive-git


Description
---

In the final version the prepDb creates the transactional tables with the init 
schema (and ignores the others if they exists). The cleanDb resets the database 
to the starting point. So between the test cases the cleanDb call is enough. If 
the prepDb is called unneccessary it will just check if the txns table exist 
and then return, so it will be fast


Diffs (updated)
-

  contrib/src/test/queries/clientpositive/url_hook.q 512e579fb8 
  itests/hive-blobstore/pom.xml 09955c55f3 
  itests/qtest-accumulo/pom.xml a35d2a8a10 
  itests/qtest-druid/pom.xml cc0cceff68 
  itests/qtest-kudu/pom.xml f23399fa37 
  itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliAdapter.java 
fcfc79059a 
  
itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java
 5b08f8b894 
  
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestMetaStoreHandler.java 
b86d736a89 
  itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 38e4bac2cc 
  pom.xml b29c06c69e 
  ql/pom.xml a0e77a1d32 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
1d211857bf 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandlerNoConnectionPool.java
 ebe4880e3a 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/ITestDbTxnManager.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
73d3b91585 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java
 3e56ad513c 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 bb29410e7d 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 e7910c1c5d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 5f3db52c2f 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Mysql.java
 c537d95470 


Diff: https://reviews.apache.org/r/72380/diff/2/

Changes: https://reviews.apache.org/r/72380/diff/1-2/


Testing
---

On my machine the 50 tests in TestDbTxnManager2 on postgres runs under 5 
minutes.


Thanks,

Peter Varga

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

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

(Updated April 21, 2020, 7:53 a.m.)


Review request for hive and Peter Vary.


Repository: hive-git


Description
---

* Use sequences for TXN_ID generation.
  * Make it possible to open transactions in parallel
  * drop Oracle 11g support


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 37a5862791 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
1d211857bf 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
6525ffc00a 
  ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 1435269ed3 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
73d3b91585 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerIsolationProperties.java
 PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
1151466f8c 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 a874121e12 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
 49b737ecf9 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 2344c2d5f6 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 bb29410e7d 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 e7910c1c5d 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
 87130a519d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
 8a3cd56658 
  
standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
 2e0177723d 
  
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
 9f3951575b 
  
standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
 0512a45cad 
  
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
 4b82e36ab4 
  
standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
 db398e5f66 
  
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
 1be83fc4a9 
  
standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
 e6e30160af 
  
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
 b90cecb173 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
 c1a1629548 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
 a6d22d19ef 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
 0b070e19ac 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72388/diff/2/

Changes: https://reviews.apache.org/r/72388/diff/1-2/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > This is a really big/scary change. I am really interested in the 
> > performance results! :)
> > Thanks for all the effort! Some questions below

Thank you for the review, i fixed most of them, but have a few questions.


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Lines 563 (patched)
> > 
> >
> > Maybe insert the query instead of getting id and using again. Not very 
> > important, just asking...

I tried it at first, but it become really slow, when there are 5 txns in 
the table. Probably there is a way to do it properly, but this is running in a 
scheduled thread so it is not that essential I think.


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 448 (original), 528 (patched)
> > 
> >
> > Just a question: This is so similar to getOpenTxnsInfo... Any way to 
> > use the same code with different query and different response handling?

I started it but I think we should do it in a next change, we have to create 
some inner datastructure, because both of responses now missing something from 
the other


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 699 (patched)
> > 
> >
> > openTxns(dbConn, stmt, rqst) is used in replTableWriteIdState as well. 
> > Do we have to check there for timeout as well?

This is an alarming catch. What kind of queries run on a replication. I put 
there the table lock, but it will be a rather long synchronized block. On the 
other hand aren't the replication tasks somehow serialized? If there are no 
select queries on replication the timeout is not needed, if there are, we might 
have a problem, because we do a lot of think before the commit, where we can 
measure.
And the most seriuos problem, if I see correctly, the replication will use this 
newly opened transactionId-s to write into the TXN_TO_WRITE_ID, wouldn't be a 
problem if this will be different than the original value?


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 780 (patched)
> > 
> >
> > TXN_META_INFO? What is it used for before? Or is it new? Could we use a 
> > specific "state" for example?
> 
> Denys Kuzmenko wrote:
> My understanding was, it's kinda marker to flag the new txn entries and 
> get their ids.

It is a marker, I just used an existing field. It is updated to null later.


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 641 (original), 783 (patched)
> > 
> >
> > Could we use a correct PreparedStatement, with the values set without 
> > the "hacky" quoting?
> > 
> > Like: pstmt.setXX for state/user/host/type/metainfo?
> > 
> > Or does this have a noticable performance gain?

Changed the metainfo select back and update to preparedstatements. The original 
insert will entirely change in your batch insert patch if I know correctly, so 
I will leave that.


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 1168 (original), 1418 (patched)
> > 
> >
> > If we do not make such a fuss about the checking, just a simple assert 
> > instead, we might can inline the method here, since it is not used anywhere 
> > else? What do you think?

The commitTxns is already 200 lines long, if it's ok for you, i woudn't make it 
longer


> On April 20, 2020, 12:19 p.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 2262 (patched)
> > 
> >
> > This is again a single query implemented with 3 SQL query and a java 
> > code. Am I right?

Yes, however it is a backgroung method. Does it worth to sacrifice the easier 
logging and debugging for some performance gain?


- Peter


---
This is an automatically generated e-mail. To 

Re: Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > LGTM, just a few comments

Thank you for the review, I fixed most of the issues, but have a few question.


> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 552 (original), 560 (patched)
> > 
> >
> > Could we use this query as condition for delete? selete where exists 
> > (..)

I would rather not do that, this method is called from a scheduled backgroung 
thread, it don't have to be incredible fast, on the other hand, if we run it 
this way we log the txnid-s between the select and the delete. I would like to 
keep that.


> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 172 (original), 225 (patched)
> > 
> >
> > Please rebase, this code is no longer here

which commit removed this? I don't see it.


> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 807 (patched)
> > 
> >
> > could we use stream instead of for loop: 
> > first = 
> > txnIds.stream().max(Comparator.comparing(Long::valueOf)).orElse(LONG_MAX)

actually turned out this is a dead code since the min history rebase, removed 
it alltogether


> On April 20, 2020, 10:40 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 902 (patched)
> > 
> >
> > Can it be null? Could we add constraint on db level instead of multiple 
> > checks in backend code?

It should not be null, but the constraint is that there must be always one 
record in the table, not a nonnull column.


- Peter


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


On April 20, 2020, 7:25 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72388/
> ---
> 
> (Updated April 20, 2020, 7:25 a.m.)
> 
> 
> Review request for hive and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> * Use sequences for TXN_ID generation.
>   * Make it possible to open transactions in parallel
>   * drop Oracle 11g support
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 
> 37a5862791 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> 1d211857bf 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
>   ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
> 6525ffc00a 
>   ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 
> 1435269ed3 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 73d3b91585 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerAcid.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
> 1151466f8c 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  f6097f7520 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
>  49b737ecf9 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  2344c2d5f6 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  bb29410e7d 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  d080df417b 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
>  87130a519d 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
>  8a3cd56658 
>   
> standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
>  

Review Request 72388: HIVE-23048 Use sequences for TXN_ID generation

[Peter Varga via Review Board]

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

Review request for hive and Peter Vary.


Repository: hive-git


Description
---

* Use sequences for TXN_ID generation.
  * Make it possible to open transactions in parallel
  * drop Oracle 11g support


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java 37a5862791 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
1d211857bf 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f 
  ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java 
6525ffc00a 
  ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java 1435269ed3 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
73d3b91585 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerAcid.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java 
1151466f8c 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 f6097f7520 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java
 49b737ecf9 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 2344c2d5f6 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 bb29410e7d 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 d080df417b 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
 87130a519d 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql
 8a3cd56658 
  
standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql
 2e0177723d 
  
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql
 9f3951575b 
  
standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql
 0512a45cad 
  
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql
 4b82e36ab4 
  
standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql
 db398e5f66 
  
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql
 1be83fc4a9 
  
standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql
 e6e30160af 
  
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql
 b90cecb173 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java
 c1a1629548 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java
 a6d22d19ef 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java
 0b070e19ac 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/72388/diff/1/


Testing
---


Thanks,

Peter Varga

Re: Review Request 72380: HIVE-23207 Create integration tests for TxnManager for different rdbms metastores

[Peter Varga via Review Board]

> On April 17, 2020, 7:03 p.m., Peter Vary wrote:
> > Thanks Peter for the patch!
> > This fix is long overdue!
> > 
> > I do not understand one thing, see below.
> > 
> > Also I would like to ask Denys to confirm, that running the init sqls again 
> > and again will not cause too much overhead in test runtime as he mentioned 
> > in our discussion. (5 min tests are fine, 1 hour tests are not fine :))
> > 
> > Thanks!

In this version we do not run the init sql again and again. We just run it 
once, and we just run the cleanDb between tests. There might be some Test 
classes that is not true, because there are multiple prepDb calls (I solved in 
in Qtest, TestDbTxnManager2) but in those places it will be just one call to 
check if the txns table exists, and then we return


- Peter


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


On April 17, 2020, 3:24 p.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72380/
> ---
> 
> (Updated April 17, 2020, 3:24 p.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Zoltan Chovan.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> In the final version the prepDb creates the transactional tables with the 
> init schema (and ignores the others if they exists). The cleanDb resets the 
> database to the starting point. So between the test cases the cleanDb call is 
> enough. If the prepDb is called unneccessary it will just check if the txns 
> table exist and then return, so it will be fast
> 
> 
> Diffs
> -
> 
>   itests/hive-blobstore/pom.xml 09955c55f3 
>   itests/qtest-accumulo/pom.xml a35d2a8a10 
>   itests/qtest-druid/pom.xml cc0cceff68 
>   itests/qtest-kudu/pom.xml f23399fa37 
>   
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestMetaStoreHandler.java
>  b86d736a89 
>   pom.xml 90e39702a1 
>   ql/pom.xml d1846c9245 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  15fcfc0e35 
>   ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
> 1d211857bf 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandlerNoConnectionPool.java
>  ebe4880e3a 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/ITestDbTxnManager.java 
> PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 73d3b91585 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java
>  3e56ad513c 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
>  a66e16973f 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  962a63d418 
>   
> standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
>  1ace9d3ef0 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
>  5f3db52c2f 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Mysql.java
>  c537d95470 
> 
> 
> Diff: https://reviews.apache.org/r/72380/diff/1/
> 
> 
> Testing
> ---
> 
> On my machine the 50 tests in TestDbTxnManager2 on postgres runs under 5 
> minutes.
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Review Request 72380: HIVE-23207 Create integration tests for TxnManager for different rdbms metastores

[Peter Varga via Review Board]

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

Review request for hive, Denys Kuzmenko and Zoltan Chovan.


Repository: hive-git


Description
---

In the final version the prepDb creates the transactional tables with the init 
schema (and ignores the others if they exists). The cleanDb resets the database 
to the starting point. So between the test cases the cleanDb call is enough. If 
the prepDb is called unneccessary it will just check if the txns table exist 
and then return, so it will be fast


Diffs
-

  itests/hive-blobstore/pom.xml 09955c55f3 
  itests/qtest-accumulo/pom.xml a35d2a8a10 
  itests/qtest-druid/pom.xml cc0cceff68 
  itests/qtest-kudu/pom.xml f23399fa37 
  
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestMetaStoreHandler.java 
b86d736a89 
  pom.xml 90e39702a1 
  ql/pom.xml d1846c9245 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
15fcfc0e35 
  ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 
1d211857bf 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandlerNoConnectionPool.java
 ebe4880e3a 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/ITestDbTxnManager.java 
PRE-CREATION 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
73d3b91585 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java
 3e56ad513c 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java
 a66e16973f 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
 962a63d418 
  
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql
 1ace9d3ef0 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTxns.java
 5f3db52c2f 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Mysql.java
 c537d95470 


Diff: https://reviews.apache.org/r/72380/diff/1/


Testing
---

On my machine the 50 tests in TestDbTxnManager2 on postgres runs under 5 
minutes.


Thanks,

Peter Varga

Re: Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

[Peter Varga via Review Board]

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

(Updated April 6, 2020, 10:04 a.m.)


Review request for hive and Adam Szita.


Repository: hive-git


Description
---

KILL  command was implemented in:

https://issues.apache.org/jira/browse/HIVE-17483
https://issues.apache.org/jira/browse/HIVE-20549
But it is not working in an environment where service discovery is enabled and 
more than one HS2 instance is running (except for manually sending the kill 
query to all HS2 instance).

Solution:

If a HS2 instance can't kill a query locally, it should post a kill query 
request to the Zookeeper
Every HS2 should watch the Zookeeper for kill query requests and if its running 
on that instance kill it
Authorization of kill query should work the same


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 73f185a1f3 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
3973ec9270 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
 68a515ccbe 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
 PRE-CREATION 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
 99e681e5b2 
  
itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
 PRE-CREATION 
  itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
1b60a51ebd 
  jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java db965e7a22 
  
ql/src/java/org/apache/hadoop/hive/ql/ddl/process/kill/KillQueriesOperation.java
 afde1a4762 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
8becef1cd3 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
9e497545b5 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
277519cba5 
  service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
  service/src/java/org/apache/hive/service/server/KillQueryImpl.java 883e32bd2e 
  
service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java 
PRE-CREATION 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
 71d8651712 


Diff: https://reviews.apache.org/r/72276/diff/3/

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


Testing
---


Thanks,

Peter Varga

Re: Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

[Peter Varga via Review Board]

> On March 30, 2020, 9:38 a.m., Adam Szita wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
> > Lines 94 (patched)
> > 
> >
> > Can be private if not used elsewhere

I reverted this one, since its needed during the query execution and must be 
public. Now every tests passes.


- Peter


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


On March 27, 2020, 10:08 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72276/
> ---
> 
> (Updated March 27, 2020, 10:08 a.m.)
> 
> 
> Review request for hive and Adam Szita.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> KILL  command was implemented in:
> 
> https://issues.apache.org/jira/browse/HIVE-17483
> https://issues.apache.org/jira/browse/HIVE-20549
> But it is not working in an environment where service discovery is enabled 
> and more than one HS2 instance is running (except for manually sending the 
> kill query to all HS2 instance).
> 
> Solution:
> 
> If a HS2 instance can't kill a query locally, it should post a kill query 
> request to the Zookeeper
> Every HS2 should watch the Zookeeper for kill query requests and if its 
> running on that instance kill it
> Authorization of kill query should work the same
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 34df01e60e 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
> 3973ec9270 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
>  68a515ccbe 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
>  PRE-CREATION 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
>  99e681e5b2 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
>  PRE-CREATION 
>   itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
> 1b60a51ebd 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java db965e7a22 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/process/kill/KillQueriesOperation.java
>  afde1a4762 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 8becef1cd3 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 9e497545b5 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> 277519cba5 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
>   service/src/java/org/apache/hive/service/server/KillQueryImpl.java 
> 883e32bd2e 
>   
> service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
>  71d8651712 
> 
> 
> Diff: https://reviews.apache.org/r/72276/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

[Peter Varga via Review Board]

> On March 30, 2020, 9:38 a.m., Adam Szita wrote:
> > Looking pretty good overall, I just have a few questions/comments.

I fixed the issues, added a little more logging and fixed the weird annotation 
formatting.


> On March 30, 2020, 9:38 a.m., Adam Szita wrote:
> > service/src/java/org/apache/hive/service/server/KillQueryImpl.java
> > Line 150 (original), 176 (patched)
> > 
> >
> > Shouldn't we return if there are no ops to kill? I think a subsequent 
> > killOperations() call here might throw an NPE.

We can not return otherwise the remote kill would not happen. The 
killOperations is not called, when the query is not found, I made the code more 
readable to reflect that.


> On March 30, 2020, 9:38 a.m., Adam Szita wrote:
> > service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
> > Lines 449 (patched)
> > 
> >
> > Shouldn't we clear the progress flag here?

We break out the loop immediatly after this. The progress flag is not used 
after that.


- Peter


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


On March 27, 2020, 10:08 a.m., Peter Varga wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72276/
> ---
> 
> (Updated March 27, 2020, 10:08 a.m.)
> 
> 
> Review request for hive and Adam Szita.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> KILL  command was implemented in:
> 
> https://issues.apache.org/jira/browse/HIVE-17483
> https://issues.apache.org/jira/browse/HIVE-20549
> But it is not working in an environment where service discovery is enabled 
> and more than one HS2 instance is running (except for manually sending the 
> kill query to all HS2 instance).
> 
> Solution:
> 
> If a HS2 instance can't kill a query locally, it should post a kill query 
> request to the Zookeeper
> Every HS2 should watch the Zookeeper for kill query requests and if its 
> running on that instance kill it
> Authorization of kill query should work the same
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 34df01e60e 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
> 3973ec9270 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
>  68a515ccbe 
>   
> itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
>  PRE-CREATION 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
>  99e681e5b2 
>   
> itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
>  PRE-CREATION 
>   itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
> 1b60a51ebd 
>   jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java db965e7a22 
>   
> ql/src/java/org/apache/hadoop/hive/ql/ddl/process/kill/KillQueriesOperation.java
>  afde1a4762 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
> 8becef1cd3 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 9e497545b5 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> 277519cba5 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
>   service/src/java/org/apache/hive/service/server/KillQueryImpl.java 
> 883e32bd2e 
>   
> service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java
>  PRE-CREATION 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
>  71d8651712 
> 
> 
> Diff: https://reviews.apache.org/r/72276/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Review Request 72276: HIVE-23084: Implement kill query in multiple HS2 environment

[Peter Varga via Review Board]

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

Review request for hive and Adam Szita.


Repository: hive-git


Description
---

KILL  command was implemented in:

https://issues.apache.org/jira/browse/HIVE-17483
https://issues.apache.org/jira/browse/HIVE-20549
But it is not working in an environment where service discovery is enabled and 
more than one HS2 instance is running (except for manually sending the kill 
query to all HS2 instance).

Solution:

If a HS2 instance can't kill a query locally, it should post a kill query 
request to the Zookeeper
Every HS2 should watch the Zookeeper for kill query requests and if its running 
on that instance kill it
Authorization of kill query should work the same


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 34df01e60e 
  itests/hive-unit/src/test/java/org/apache/hive/jdbc/BaseJdbcWithMiniLlap.java 
3973ec9270 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniLlapArrow.java
 68a515ccbe 
  
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithServiceDiscovery.java
 PRE-CREATION 
  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestMiniHS2StateWithNoZookeeper.java
 99e681e5b2 
  
itests/hive-unit/src/test/java/org/apache/hive/service/server/TestKillQueryZookeeperManager.java
 PRE-CREATION 
  itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 
1b60a51ebd 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 
8becef1cd3 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
9e497545b5 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
277519cba5 
  service/src/java/org/apache/hive/service/server/HiveServer2.java 181ea5d6d5 
  service/src/java/org/apache/hive/service/server/KillQueryImpl.java 883e32bd2e 
  
service/src/java/org/apache/hive/service/server/KillQueryZookeeperManager.java 
PRE-CREATION 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
 71d8651712 


Diff: https://reviews.apache.org/r/72276/diff/1/


Testing
---


Thanks,

Peter Varga

Review Request 72246: HIVE-23045: Zookeeper SSL/TLS support

[Peter Varga via Review Board]

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

Review request for hive, Denys Kuzmenko and Peter Vary.


Repository: hive-git


Description
---

Enable SSL communication with Zookeeper in Hive


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d50912b4e2 
  
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/ZooKeeperStorage.java
 1fc8d36394 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/security/TestZooKeeperTokenStore.java
 603155bf8f 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/security/TestZookeeperTokenStoreSSLEnabled.java
 PRE-CREATION 
  
itests/hive-unit/src/test/java/org/apache/hive/service/server/TestInformationSchemaWithPrivilege.java
 de2e4937a8 
  
itests/hive-unit/src/test/java/org/apache/hive/service/server/TestInformationSchemaWithPriviligeZookeeperSSL.java
 PRE-CREATION 
  jdbc/src/java/org/apache/hive/jdbc/Utils.java e23826eb56 
  jdbc/src/java/org/apache/hive/jdbc/ZooKeeperHiveClientHelper.java 759ba8a5ef 
  llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java 
d28fd1778c 
  
ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/CuratorFrameworkSingleton.java
 fa3a382367 
  ql/src/test/org/apache/hive/testutils/MiniZooKeeperCluster.java eec628263a 
  service/src/java/org/apache/hive/service/server/HiveServer2.java fece82e266 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/SSLZookeeperFactory.java
 PRE-CREATION 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java
 99f7c97877 
  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 fc6a2fd43a 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/security/MetastoreDelegationTokenManager.java
 b35dc7ce4b 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/security/ZooKeeperTokenStore.java
 af52fcc5f6 


Diff: https://reviews.apache.org/r/72246/diff/1/


Testing
---


Thanks,

Peter Varga