You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Varga via Review Board <no...@reviews.apache.org> on 2020/05/04 07:35:05 UTC

Review Request 72465: HIVE-23340 TxnHandler cleanup

-----------------------------------------------------------
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 72465: HIVE-23340 TxnHandler cleanup

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/72465/diff/2/?file=2230140#file2230140line400>
> >
> >     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?

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


- Denys


-----------------------------------------------------------
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 72465: HIVE-23340 TxnHandler cleanup

Posted by Peter Varga via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/72465/diff/2/?file=2230140#file2230140line400>
> >
> >     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 72465: HIVE-23340 TxnHandler cleanup

Posted by Peter Varga via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/72465/diff/2/?file=2230138#file2230138line111>
> >
> >     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)
> > <https://reviews.apache.org/r/72465/diff/2/?file=2230140#file2230140line400>
> >
> >     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

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72465/#review220593
-----------------------------------------------------------




standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 107 (original), 110 (patched)
<https://reviews.apache.org/r/72465/#comment309071>

    Could we add helper methods into the enum to avoid calling quoteChar everywhere, like TxnSatus.aborted()?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 237 (original), 238 (patched)
<https://reviews.apache.org/r/72465/#comment309070>

    By adding TxnState as 2nd argument, you can simplify toTxnState()
    Also could we extract this enum to util package or something, TxnHandler is massive enough.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Lines 271 (patched)
<https://reviews.apache.org/r/72465/#comment309069>

    Better create and use reverse lookup map, current approach not really maintainable.
    
    Map<String, TxnStatus> LOOKUP = Maps.uniqueIndex(
        Arrays.asList(TxnStatus.values()),
        MyEnum::getValue
    );



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Lines 495 (patched)
<https://reviews.apache.org/r/72465/#comment309075>

    Could we reuse TxnInfo instead of OpenTxn? We won't have to iterate again.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 464 (original), 551 (patched)
<https://reviews.apache.org/r/72465/#comment309074>

    Could we have 2 constants for query with and without info fields instead of constructing them here?



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 1093 (original), 1094 (patched)
<https://reviews.apache.org/r/72465/#comment309073>

    1 line predicate is much more readable, at least for me



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnUtils.java
Lines 400 (patched)
<https://reviews.apache.org/r/72465/#comment309072>

    Is is a generic txn object (i.e TxnInfo)? I don't think TxnUtils is a good place for domain objects.


- Denys Kuzmenko


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 72465: HIVE-23340 TxnHandler cleanup

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72465/#review220950
-----------------------------------------------------------


Ship it!




Ship It!

- Denys Kuzmenko


On May 20, 2020, 12:24 p.m., Peter Varga wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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 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/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

Posted by Peter Varga via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/72465/diff/3/?file=2230714#file2230714line32>
> >
> >     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
> 
> Peter Varga wrote:
>     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.

ok, let's address this in diff patch


- Denys


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


On May 20, 2020, 12:24 p.m., Peter Varga wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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 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/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.

- Denys


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


On May 20, 2020, 12:24 p.m., Peter Varga wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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 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/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Peter Varga
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

Posted by Peter Varga via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/72465/diff/3/?file=2230714#file2230714line32>
> >
> >     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
> 
>

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.

> 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)
> > <https://reviews.apache.org/r/72465/diff/3/?file=2230714#file2230714line32>
> >
> >     Could you please refactor here as well LockTypeUtil to LockType enum.

move to txn package + overwrite toString(), this would allow to get rid of static exclWrite, sharedWrite, sharedRead methods


- Denys


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

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72465/#review220802
-----------------------------------------------------------



Thank you for the patch, Peter! LGTM 
1 comment on static import + 1 ask to refactor.


standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Lines 40 (patched)
<https://reviews.apache.org/r/72465/#comment309502>

    Up to you, but I wouldn't use static import.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 1037 (original), 905 (patched)
<https://reviews.apache.org/r/72465/#comment309500>

    I wouldn't use static import here, but TxnStatus.OPEN instead to be consistent and I think it's more clear what 'OPEN' is related to (previously it was TXN_OPEN).



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStatus.java
Lines 32 (patched)
<https://reviews.apache.org/r/72465/#comment309501>

    Could you please refactor here as well LockTypeUtil to LockType enum.


- Denys Kuzmenko


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

Posted by Peter Varga via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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

Re: Review Request 72465: HIVE-23340 TxnHandler cleanup

Posted by Peter Varga via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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