> On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > This is a really big/scary change. I am really interested in the > > performance results! :) > > Thanks for all the effort! Some questions below
Thank you for the review, i fixed most of them, but have a few questions. > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java > > Lines 563 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223823#file2223823line563> > > > > Maybe insert the query instead of getting id and using again. Not very > > important, just asking... I tried it at first, but it become really slow, when there are 50000 txns in the table. Probably there is a way to do it properly, but this is running in a scheduled thread so it is not that essential I think. > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Line 448 (original), 528 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223825#file2223825line545> > > > > Just a question: This is so similar to getOpenTxnsInfo... Any way to > > use the same code with different query and different response handling? I started it but I think we should do it in a next change, we have to create some inner datastructure, because both of responses now missing something from the other > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Lines 699 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223825#file2223825line731> > > > > openTxns(dbConn, stmt, rqst) is used in replTableWriteIdState as well. > > Do we have to check there for timeout as well? This is an alarming catch. What kind of queries run on a replication. I put there the table lock, but it will be a rather long synchronized block. On the other hand aren't the replication tasks somehow serialized? If there are no select queries on replication the timeout is not needed, if there are, we might have a problem, because we do a lot of think before the commit, where we can measure. And the most seriuos problem, if I see correctly, the replication will use this newly opened transactionId-s to write into the TXN_TO_WRITE_ID, wouldn't be a problem if this will be different than the original value? > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Lines 780 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223825#file2223825line826> > > > > TXN_META_INFO? What is it used for before? Or is it new? Could we use a > > specific "state" for example? > > Denys Kuzmenko wrote: > My understanding was, it's kinda marker to flag the new txn entries and > get their ids. It is a marker, I just used an existing field. It is updated to null later. > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Line 641 (original), 783 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223825#file2223825line829> > > > > Could we use a correct PreparedStatement, with the values set without > > the "hacky" quoting? > > > > Like: pstmt.setXX for state/user/host/type/metainfo? > > > > Or does this have a noticable performance gain? Changed the metainfo select back and update to preparedstatements. The original insert will entirely change in your batch insert patch if I know correctly, so I will leave that. > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Line 1168 (original), 1418 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223825#file2223825line1466> > > > > If we do not make such a fuss about the checking, just a simple assert > > instead, we might can inline the method here, since it is not used anywhere > > else? What do you think? The commitTxns is already 200 lines long, if it's ok for you, i woudn't make it longer > On April 20, 2020, 12:19 p.m., Peter Vary wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Lines 2262 (patched) > > <https://reviews.apache.org/r/72388/diff/1/?file=2223825#file2223825line2332> > > > > This is again a single query implemented with 3 SQL query and a java > > code. Am I right? Yes, however it is a backgroung method. Does it worth to sacrifice the easier logging and debugging for some performance gain? - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72388/#review220369 ----------------------------------------------------------- On April 20, 2020, 7:25 a.m., Peter Varga wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72388/ > ----------------------------------------------------------- > > (Updated April 20, 2020, 7:25 a.m.) > > > Review request for hive and Peter Vary. > > > Repository: hive-git > > > Description > ------- > > * Use sequences for TXN_ID generation. > * Make it possible to open transactions in parallel > * drop Oracle 11g support > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java > 37a5862791 > > ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java > 15fcfc0e35 > ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java > 1d211857bf > ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 2c13e8dd03 > ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 51b0fa336f > ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommandsForMmTable.java > 6525ffc00a > ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java > 1435269ed3 > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > 73d3b91585 > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManagerAcid.java > PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestInitiator.java > 1151466f8c > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > f6097f7520 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/SQLGenerator.java > 49b737ecf9 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java > 2344c2d5f6 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDbUtil.java > bb29410e7d > > 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/TxnStore.java > 87130a519d > > standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.0.0.derby.sql > 1ace9d3ef0 > > standalone-metastore/metastore-server/src/main/sql/derby/upgrade-3.2.0-to-4.0.0.derby.sql > 8a3cd56658 > > standalone-metastore/metastore-server/src/main/sql/mssql/hive-schema-4.0.0.mssql.sql > 2e0177723d > > standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-3.2.0-to-4.0.0.mssql.sql > 9f3951575b > > standalone-metastore/metastore-server/src/main/sql/mysql/hive-schema-4.0.0.mysql.sql > 0512a45cad > > standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-3.2.0-to-4.0.0.mysql.sql > 4b82e36ab4 > > standalone-metastore/metastore-server/src/main/sql/oracle/hive-schema-4.0.0.oracle.sql > db398e5f66 > > standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-3.2.0-to-4.0.0.oracle.sql > 1be83fc4a9 > > standalone-metastore/metastore-server/src/main/sql/postgres/hive-schema-4.0.0.postgres.sql > e6e30160af > > standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-3.2.0-to-4.0.0.postgres.sql > b90cecb173 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/DbInstallBase.java > c1a1629548 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/DatabaseRule.java > a6d22d19ef > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Oracle.java > 0b070e19ac > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/txn/TestOpenTxn.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/72388/diff/1/ > > > Testing > ------- > > > Thanks, > > Peter Varga > >
