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