----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72264/#review220087 -----------------------------------------------------------
LGTM, however there are some issues to consider. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 2459 (patched) <https://reviews.apache.org/r/72264/#comment308376> I would recommend using PreparedStatements here and move query strings into the constants (see my batching patch). standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Line 2429 (original), 2470 (patched) <https://reviews.apache.org/r/72264/#comment308377> Same here. Move query string into constants. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 2505 (patched) <https://reviews.apache.org/r/72264/#comment308378> Not optimal condition, should be (insertCounter % maxInsertsPerBatch != 0) standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 2519 (patched) <https://reviews.apache.org/r/72264/#comment308379> Move query to string constant. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 2530 (patched) <https://reviews.apache.org/r/72264/#comment308380> Should we consider returning Optional here? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 2575 (patched) <https://reviews.apache.org/r/72264/#comment308382> You'll have to merge with mine again (sorry) standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 2625 (patched) <https://reviews.apache.org/r/72264/#comment308383> After this change we should update condition in checkLock query. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Line 2617 (original), 2628 (patched) <https://reviews.apache.org/r/72264/#comment308384> Is it possible to extract this one into LockType related object and make it static or introduce enum for that? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Line 3303 (original), 3323 (patched) <https://reviews.apache.org/r/72264/#comment308385> Move queryStr into constants. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 3340 (patched) <https://reviews.apache.org/r/72264/#comment308387> Why do you need to clear batch, after execute it will be empty. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 3343 (patched) <https://reviews.apache.org/r/72264/#comment308386> Not optimal condition, should be (insertCounter % maxInsertsPerBatch != 0) - Denys Kuzmenko On March 24, 2020, 2:24 p.m., Marton Bod wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72264/ > ----------------------------------------------------------- > > (Updated March 24, 2020, 2:24 p.m.) > > > Review request for hive, Denys Kuzmenko and Peter Vary. > > > Repository: hive-git > > > Description > ------- > > HIVE-23052: Optimize lock enqueueing in TxnHandler > > > Diffs > ----- > > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > 06defdb910 > > > Diff: https://reviews.apache.org/r/72264/diff/1/ > > > Testing > ------- > > Green build: https://builds.apache.org/job/PreCommit-HIVE-Build/21249/ > Custom benchmark tests with local and remote DB > > > Thanks, > > Marton Bod > >
