Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-23 Thread Denys Kuzmenko via Review Board 

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


Ship it!




Ship It!

- Denys Kuzmenko


On April 23, 2020, 8:56 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 23, 2020, 8:56 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> 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/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/7/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-23 Thread Marton Bod 

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

(Updated April 23, 2020, 8:56 a.m.)


Review request for hive, Denys Kuzmenko and Peter Vary.


Repository: hive-git


Description
---

HIVE-23201: Improve logging in locking


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
  
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/TxnUtils.java
 b3a1f826bb 


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

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


Testing
---

Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/


Thanks,

Marton Bod

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-23 Thread Marton Bod 


> On April 22, 2020, 11:46 a.m., Denys Kuzmenko wrote:
> > Looks good, however I don't really like change with 
> > REQ_LOCKS_ALIASED_FIELDS as it generats much bigger query (keep in mind 
> > there 3 of them joined by union, will be 4 for excl_write) + we are 
> > transfering more data that we are not very interested in. To get more 
> > information about the lock we can simply run show locks.

Thanks Denys. As discussed, there was a misunderstanding related to logging out 
the blocked lock in more detail. 
I've reverted that part and updated the diff.


- Marton


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


On April 17, 2020, 11:30 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 17, 2020, 11:30 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> 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/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/4/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-22 Thread Denys Kuzmenko via Review Board 

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



Looks good, however I don't really like change with REQ_LOCKS_ALIASED_FIELDS as 
it generats much bigger query (keep in mind there 3 of them joined by union, 
will be 4 for excl_write) + we are transfering more data that we are not very 
interested in. To get more information about the lock we can simply run show 
locks.

- Denys Kuzmenko


On April 17, 2020, 11:30 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 17, 2020, 11:30 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> 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/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/3/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-22 Thread Marton Bod 


> On April 21, 2020, 10:38 a.m., Denys Kuzmenko wrote:
> > LGTM, just number of comments

Thanks for the review Denys! 
As per your comments, I have elevated the log levels, removed the less useful 
log lines and changed the original statements to prepared statements.


- Marton


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


On April 17, 2020, 11:30 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 17, 2020, 11:30 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> 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/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/2/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-21 Thread Denys Kuzmenko via Review Board 


> On April 21, 2020, 10:38 a.m., Denys Kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 4427 (patched)
> > 
> >
> > I would keep as it was before. no need to modify the query

Maybe, just refrase "Failed to acquire lock: (extLockId={}, intLockId={}) as 
it's aleady taken by: (extLockId={}, intLockId={})"


- Denys


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


On April 17, 2020, 11:30 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 17, 2020, 11:30 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> 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/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/2/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-21 Thread Denys Kuzmenko via Review Board 

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



LGTM, just number of comments


ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java
Lines 143 (patched)


Maybe use log.error here?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2413 (original), 2423 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2438 (original), 2449 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2633 (original), 2644 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2679 (original), 2692 (patched)


what's the benefit of this?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2693 (original), 2707 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2747 (original), 2761 (patched)


What's the benefit of this?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2773 (original), 2787 (patched)


log.error, start with capitalized



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Lines 4427 (patched)


I would keep as it was before. no need to modify the query



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4458 (original), 4476 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4481 (original), 4492 (patched)


What's the purpose of this?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4502 (original), 4513 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4527 (original), 4536 (patched)


log.error



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4717 (original), 4726 (patched)


Why not Prepared statement?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4728 (original), 4731 (patched)


could we use Optional here?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4742 (original), 4742 (patched)


Why not Prepared statement?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4771 (original), 4764 (patched)


Why not Prepared statement?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4809 (original), 4799 (patched)


I would refrase like
Deleted {} locks due to timed-out. Lock ids: {}



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4817 (original), 4804 (patched)


Do we need "due to" if we supply exception.


- Denys Kuzmenko


On April 17, 2020, 11:30 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 17, 2020, 11:30 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-20 Thread Marton Bod 


> On April 17, 2020, 1:46 p.m., Peter Vary wrote:
> > Mostly agree, few comments.
> > I would like to ask you to go through them, and if there are any places 
> > where the problem should not happen use at least info level.
> > 
> > Thanks,
> > Peter

Thanks for the review Peter! I've made changes based on your comments


- Marton


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


On April 17, 2020, 11:30 a.m., Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated April 17, 2020, 11:30 a.m.)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> 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/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/2/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>

Re: Review Request 72378: HIVE-23201: Improve logging in locking

2020-04-17 Thread Peter Vary via Review Board 

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



Mostly agree, few comments.
I would like to ask you to go through them, and if there are any places where 
the problem should not happen use at least info level.

Thanks,
Peter


standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Lines 4492 (patched)


nit: Maybe remove the last ','?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4810 (original), 4800 (patched)


I prefer the original info level for these logs.


- Peter Vary


On ápr. 17, 2020, 11:30 de, Marton Bod wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72378/
> ---
> 
> (Updated ápr. 17, 2020, 11:30 de)
> 
> 
> Review request for hive, Denys Kuzmenko and Peter Vary.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-23201: Improve logging in locking
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbLockManager.java 4b6bc3e1e3 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
>  962a63d418 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
>  b3a1f826bb 
> 
> 
> Diff: https://reviews.apache.org/r/72378/diff/1/
> 
> 
> Testing
> ---
> 
> Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21717/
> 
> 
> Thanks,
> 
> Marton Bod
> 
>