Re: Review Request 62693: HIVE-17635: Add unit tests to CompactionTxnHandler and use PreparedStatements for queries

2017-10-13 Thread Andrew Sherman via Review Board 


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 387 (original), 411 (patched)
> > 
> >
> > What is this for?

The 'questions' list is used to create the list containing 'IN list values'. If 
not using PreparedStatements these would be actual values. In the 
PreparedStatement case they are a list of ?. This is arguably ugly but it 
allows us to use common code for PreparedStatements and (unprepared) 
Statements. See below for a more complete explanantion.


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 413 (original), 444 (patched)
> > 
> >
> > Is this necessary?

No, its a mistake, thanks


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 432 (original), 475 (patched)
> > 
> >
> > If we are changing this, should we just use try-with-resources.

I agree try-with-resources is great, I wanted to mimimize my changes


> On Oct. 12, 2017, 5:34 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
> > Lines 158 (patched)
> > 
> >
> > why is a new return value necessary?

We are looking at code that generates IN clauses: 
  select count(*) from TXNS where (TXN_ID in (1,2,3,4,5)) and TXN_STATE = 'o'
There are limits on how many values you can have in an IN clause (like maybe 
1000), and the code knows something about that.
If you ask it to generate code for a lot of values then it returns multiple 
queries:
  select count(*) from TXNS where (TXN_ID in (1,2,3,4,5)) and TXN_STATE = 'o'
  select count(*) from TXNS where (TXN_ID in (1001,1002,1003) and TXN_STATE = 
'o'
My change involves using the same logic to build PreparedStatements. These look 
like:
  select count(*) from TXNS where (TXN_ID in (?,?,?,?,?)) and TXN_STATE = 'o'
  select count(*) from TXNS where (TXN_ID in (?,?,?)) and TXN_STATE = 'o'
The difference is that with PreparedStatements the code must also subsequently 
call 
 pStmt.setLong(paramNum, value)
The right number of times for each query. So the new method 
buildQueryWithINClauseStrings,  
in addition to building the list of queries also returns a corresponding list 
of the number of ? i
n the the generated in clause.


- Andrew


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


On Sept. 29, 2017, 4:51 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62693/
> ---
> 
> (Updated Sept. 29, 2017, 4:51 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Add a unit test which exercises CompactionTxnHandler.markFailed() and change 
> it to use PreparedStament.
> Add test for checkFailedCompactions() and change it to use PreparedStatement
> Add a unit test which exercises purgeCompactionHistory().
> Add buildQueryWithINClauseStrings() which is suitable for building in clauses 
> for PreparedStatement
> Add test code to TestTxnUtils to tickle code in 
> TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
> Change markCleaned() to use PreparedStatement
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 
> 84963af10ec13979a7b3976be434efbc21cf2382 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  60839faa352cbf959041a455e9e780dfca0afdc3 
>   metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 
> 30b155f3b3311fed6cd79e46a5b2abcee9927d91 
>   metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 
> 1497c00e5dc77c02e53767b014a23e5fd8cb5b29 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  f8ae86bea3fe78374c0e0487d66c661f4f0d78ff 
> 
> 
> Diff: https://reviews.apache.org/r/62693/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 62693: HIVE-17635: Add unit tests to CompactionTxnHandler and use PreparedStatements for queries

2017-10-12 Thread Sahil Takiar 

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




metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 387 (original), 411 (patched)


What is this for?



metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 413 (original), 444 (patched)


Is this necessary?



metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 432 (original), 475 (patched)


If we are changing this, should we just use try-with-resources.



metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 158 (patched)


why is a new return value necessary?



metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 170 (patched)


nit: extra newline



metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 192 (patched)


nit: delete newline


- Sahil Takiar


On Sept. 29, 2017, 4:51 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62693/
> ---
> 
> (Updated Sept. 29, 2017, 4:51 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Add a unit test which exercises CompactionTxnHandler.markFailed() and change 
> it to use PreparedStament.
> Add test for checkFailedCompactions() and change it to use PreparedStatement
> Add a unit test which exercises purgeCompactionHistory().
> Add buildQueryWithINClauseStrings() which is suitable for building in clauses 
> for PreparedStatement
> Add test code to TestTxnUtils to tickle code in 
> TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
> Change markCleaned() to use PreparedStatement
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 
> 84963af10ec13979a7b3976be434efbc21cf2382 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
>  60839faa352cbf959041a455e9e780dfca0afdc3 
>   metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 
> 30b155f3b3311fed6cd79e46a5b2abcee9927d91 
>   metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 
> 1497c00e5dc77c02e53767b014a23e5fd8cb5b29 
>   
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
>  f8ae86bea3fe78374c0e0487d66c661f4f0d78ff 
> 
> 
> Diff: https://reviews.apache.org/r/62693/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Review Request 62693: HIVE-17635: Add unit tests to CompactionTxnHandler and use PreparedStatements for queries

2017-09-29 Thread Andrew Sherman 

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

Review request for hive.


Repository: hive-git


Description
---

Add a unit test which exercises CompactionTxnHandler.markFailed() and change it 
to use PreparedStament.
Add test for checkFailedCompactions() and change it to use PreparedStatement
Add a unit test which exercises purgeCompactionHistory().
Add buildQueryWithINClauseStrings() which is suitable for building in clauses 
for PreparedStatement
Add test code to TestTxnUtils to tickle code in 
TxnUtils.buildQueryWithINClauseStrings() so that it produces multiple queries.
Change markCleaned() to use PreparedStatement


Diffs
-

  beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java 
84963af10ec13979a7b3976be434efbc21cf2382 
  
metastore/src/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
 60839faa352cbf959041a455e9e780dfca0afdc3 
  metastore/src/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java 
30b155f3b3311fed6cd79e46a5b2abcee9927d91 
  metastore/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnUtils.java 
1497c00e5dc77c02e53767b014a23e5fd8cb5b29 
  
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java 
f8ae86bea3fe78374c0e0487d66c661f4f0d78ff 


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


Testing
---


Thanks,

Andrew Sherman