You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Marton Bod <ma...@gmail.com> on 2020/05/07 15:55:44 UTC
Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72481/
-----------------------------------------------------------
Review request for hive, Denys Kuzmenko and Peter Vary.
Repository: hive-git
Description
-------
Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
Diffs
-----
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
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/TxnHandler.java cf41ef8aaf
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
Diff: https://reviews.apache.org/r/72481/diff/1/
Testing
-------
Unit test in TestTxnHandler
+ Perf tests:
dbType sameTable variant ms/op error
MYSQL FALSE original 46.93 3.041
MYSQL FALSE patched 19.283 1.311
MYSQL TRUE original 50.185 3.595
MYSQL TRUE patched 32.254 2.164
ORACLE FALSE original 57.609 4.461
ORACLE FALSE patched 25.721 2.551
ORACLE TRUE original 59.668 3.172
ORACLE TRUE patched 39.061 2.548
POSTGRES FALSE original 39.364 2.94
POSTGRES FALSE patched 18.518 1.038
POSTGRES TRUE original 39.868 2.679
POSTGRES TRUE patched 28.874 1.768
SQLSERVER FALSE original 45.252 1.643
SQLSERVER FALSE patched 24.583 1.529
SQLSERVER TRUE original 49.149 3.45
SQLSERVER TRUE patched 32.918 1.654
(sameTable=true means that all threads were trying to allocate ids for the same db.table,
false means they all targeted different tables)
Thanks,
Marton Bod
Re: Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
Posted by Denys Kuzmenko via Review Board <no...@reviews.apache.org>.
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 2067 (original), 2057 (patched)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230754#file2230754line2071>
> >
> > Why is this change?
>
> Marton Bod wrote:
> this was causing a checkstyle issue (line lenght too long)
it doesn't look long, maybe you can remove some leading spaces
- Denys
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72481/#review220689
-----------------------------------------------------------
On May 7, 2020, 3:55 p.m., Marton Bod wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72481/
> -----------------------------------------------------------
>
> (Updated May 7, 2020, 3:55 p.m.)
>
>
> Review request for hive, Denys Kuzmenko and Peter Vary.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
>
> The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
>
>
> Diffs
> -----
>
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
> 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/TxnHandler.java cf41ef8aaf
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
>
>
> Diff: https://reviews.apache.org/r/72481/diff/1/
>
>
> Testing
> -------
>
> Unit test in TestTxnHandler
> + Perf tests:
> dbType sameTable variant ms/op error
> MYSQL FALSE original 46.93 3.041
> MYSQL FALSE patched 19.283 1.311
> MYSQL TRUE original 50.185 3.595
> MYSQL TRUE patched 32.254 2.164
> ORACLE FALSE original 57.609 4.461
> ORACLE FALSE patched 25.721 2.551
> ORACLE TRUE original 59.668 3.172
> ORACLE TRUE patched 39.061 2.548
> POSTGRES FALSE original 39.364 2.94
> POSTGRES FALSE patched 18.518 1.038
> POSTGRES TRUE original 39.868 2.679
> POSTGRES TRUE patched 28.874 1.768
> SQLSERVER FALSE original 45.252 1.643
> SQLSERVER FALSE patched 24.583 1.529
> SQLSERVER TRUE original 49.149 3.45
> SQLSERVER TRUE patched 32.918 1.654
> (sameTable=true means that all threads were trying to allocate ids for the same db.table,
> false means they all targeted different tables)
>
>
> Thanks,
>
> Marton Bod
>
>
Re: Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
Posted by Marton Bod <ma...@gmail.com>.
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > Thanks Marci,
> > Few querstions below - probably I just do not understand this part of the code enough.
> >
> > Another question for the perf test: How many threads are you using?
> >
> > Thanks,
> > Peter
Thanks Peti for the review. See my answers below, they are related to minor housekeeping changes that are not core to the optimization story.
The perf tests were run with 8 concurrent threads.
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 1102 (original)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230753#file2230753line1102>
> >
> > Why did we remove this?
checkRetryable never throws MetaException (and hence the code never enters this catch block), so I removed it from its throws clause
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
> > Line 1137 (original)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230753#file2230753line1137>
> >
> > why did we remove this?
same as above
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 1762-1765 (original), 1759-1762 (patched)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230754#file2230754line1764>
> >
> > Is this a functionality or performance change?
neither really, just some readability refactor
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 2021 (original), 2013 (patched)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230754#file2230754line2025>
> >
> > Why is this change required?
just seems counterintuitive to use string concat if we're already using a stringbuilder anyway
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 2067 (original), 2057 (patched)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230754#file2230754line2071>
> >
> > Why is this change?
this was causing a checkstyle issue (line lenght too long)
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 2079 (original), 2070 (patched)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230754#file2230754line2084>
> >
> > Why is this change?
unnecessary call to Long.toString
> On May 8, 2020, 10:23 a.m., Peter Vary wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Line 2090 (original), 2081 (patched)
> > <https://reviews.apache.org/r/72481/diff/1/?file=2230754#file2230754line2095>
> >
> > Why is this change?
same as above
- Marton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72481/#review220689
-----------------------------------------------------------
On May 7, 2020, 3:55 p.m., Marton Bod wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72481/
> -----------------------------------------------------------
>
> (Updated May 7, 2020, 3:55 p.m.)
>
>
> Review request for hive, Denys Kuzmenko and Peter Vary.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
>
> The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
>
>
> Diffs
> -----
>
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
> 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/TxnHandler.java cf41ef8aaf
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
>
>
> Diff: https://reviews.apache.org/r/72481/diff/1/
>
>
> Testing
> -------
>
> Unit test in TestTxnHandler
> + Perf tests:
> dbType sameTable variant ms/op error
> MYSQL FALSE original 46.93 3.041
> MYSQL FALSE patched 19.283 1.311
> MYSQL TRUE original 50.185 3.595
> MYSQL TRUE patched 32.254 2.164
> ORACLE FALSE original 57.609 4.461
> ORACLE FALSE patched 25.721 2.551
> ORACLE TRUE original 59.668 3.172
> ORACLE TRUE patched 39.061 2.548
> POSTGRES FALSE original 39.364 2.94
> POSTGRES FALSE patched 18.518 1.038
> POSTGRES TRUE original 39.868 2.679
> POSTGRES TRUE patched 28.874 1.768
> SQLSERVER FALSE original 45.252 1.643
> SQLSERVER FALSE patched 24.583 1.529
> SQLSERVER TRUE original 49.149 3.45
> SQLSERVER TRUE patched 32.918 1.654
> (sameTable=true means that all threads were trying to allocate ids for the same db.table,
> false means they all targeted different tables)
>
>
> Thanks,
>
> Marton Bod
>
>
Re: Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72481/#review220689
-----------------------------------------------------------
Thanks Marci,
Few querstions below - probably I just do not understand this part of the code enough.
Another question for the perf test: How many threads are you using?
Thanks,
Peter
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 1102 (original)
<https://reviews.apache.org/r/72481/#comment309295>
Why did we remove this?
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
Line 1137 (original)
<https://reviews.apache.org/r/72481/#comment309294>
why did we remove this?
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Lines 1762-1765 (original), 1759-1762 (patched)
<https://reviews.apache.org/r/72481/#comment309290>
Is this a functionality or performance change?
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2021 (original), 2013 (patched)
<https://reviews.apache.org/r/72481/#comment309289>
Why is this change required?
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2067 (original), 2057 (patched)
<https://reviews.apache.org/r/72481/#comment309291>
Why is this change?
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2079 (original), 2070 (patched)
<https://reviews.apache.org/r/72481/#comment309292>
Why is this change?
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2090 (original), 2081 (patched)
<https://reviews.apache.org/r/72481/#comment309293>
Why is this change?
- Peter Vary
On máj. 7, 2020, 3:55 du, Marton Bod wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72481/
> -----------------------------------------------------------
>
> (Updated máj. 7, 2020, 3:55 du)
>
>
> Review request for hive, Denys Kuzmenko and Peter Vary.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
>
> The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
>
>
> Diffs
> -----
>
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
> 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/TxnHandler.java cf41ef8aaf
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
>
>
> Diff: https://reviews.apache.org/r/72481/diff/1/
>
>
> Testing
> -------
>
> Unit test in TestTxnHandler
> + Perf tests:
> dbType sameTable variant ms/op error
> MYSQL FALSE original 46.93 3.041
> MYSQL FALSE patched 19.283 1.311
> MYSQL TRUE original 50.185 3.595
> MYSQL TRUE patched 32.254 2.164
> ORACLE FALSE original 57.609 4.461
> ORACLE FALSE patched 25.721 2.551
> ORACLE TRUE original 59.668 3.172
> ORACLE TRUE patched 39.061 2.548
> POSTGRES FALSE original 39.364 2.94
> POSTGRES FALSE patched 18.518 1.038
> POSTGRES TRUE original 39.868 2.679
> POSTGRES TRUE patched 28.874 1.768
> SQLSERVER FALSE original 45.252 1.643
> SQLSERVER FALSE patched 24.583 1.529
> SQLSERVER TRUE original 49.149 3.45
> SQLSERVER TRUE patched 32.918 1.654
> (sameTable=true means that all threads were trying to allocate ids for the same db.table,
> false means they all targeted different tables)
>
>
> Thanks,
>
> Marton Bod
>
>
Re: Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
Posted by Marton Bod <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72481/
-----------------------------------------------------------
(Updated May 14, 2020, 3:38 p.m.)
Review request for hive, Denys Kuzmenko and Peter Vary.
Repository: hive-git
Description
-------
Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
Diffs (updated)
-----
ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
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/TxnHandler.java cf41ef8aaf
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
Diff: https://reviews.apache.org/r/72481/diff/2/
Changes: https://reviews.apache.org/r/72481/diff/1-2/
Testing
-------
Unit test in TestTxnHandler
+ Perf tests:
dbType sameTable variant ms/op error
MYSQL FALSE original 46.93 3.041
MYSQL FALSE patched 19.283 1.311
MYSQL TRUE original 50.185 3.595
MYSQL TRUE patched 32.254 2.164
ORACLE FALSE original 57.609 4.461
ORACLE FALSE patched 25.721 2.551
ORACLE TRUE original 59.668 3.172
ORACLE TRUE patched 39.061 2.548
POSTGRES FALSE original 39.364 2.94
POSTGRES FALSE patched 18.518 1.038
POSTGRES TRUE original 39.868 2.679
POSTGRES TRUE patched 28.874 1.768
SQLSERVER FALSE original 45.252 1.643
SQLSERVER FALSE patched 24.583 1.529
SQLSERVER TRUE original 49.149 3.45
SQLSERVER TRUE patched 32.918 1.654
(sameTable=true means that all threads were trying to allocate ids for the same db.table,
false means they all targeted different tables)
Thanks,
Marton Bod
Re: Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
Posted by Marton Bod <ma...@gmail.com>.
> On May 14, 2020, 3:07 p.m., Denys Kuzmenko wrote:
> > LGTM, some minor comments
Thanks Denys, I've address your comments
- Marton
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72481/#review220758
-----------------------------------------------------------
On May 14, 2020, 3:38 p.m., Marton Bod wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72481/
> -----------------------------------------------------------
>
> (Updated May 14, 2020, 3:38 p.m.)
>
>
> Review request for hive, Denys Kuzmenko and Peter Vary.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
>
> The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
>
>
> Diffs
> -----
>
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
> 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/TxnHandler.java cf41ef8aaf
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
>
>
> Diff: https://reviews.apache.org/r/72481/diff/2/
>
>
> Testing
> -------
>
> Unit test in TestTxnHandler
> + Perf tests:
> dbType sameTable variant ms/op error
> MYSQL FALSE original 46.93 3.041
> MYSQL FALSE patched 19.283 1.311
> MYSQL TRUE original 50.185 3.595
> MYSQL TRUE patched 32.254 2.164
> ORACLE FALSE original 57.609 4.461
> ORACLE FALSE patched 25.721 2.551
> ORACLE TRUE original 59.668 3.172
> ORACLE TRUE patched 39.061 2.548
> POSTGRES FALSE original 39.364 2.94
> POSTGRES FALSE patched 18.518 1.038
> POSTGRES TRUE original 39.868 2.679
> POSTGRES TRUE patched 28.874 1.768
> SQLSERVER FALSE original 45.252 1.643
> SQLSERVER FALSE patched 24.583 1.529
> SQLSERVER TRUE original 49.149 3.45
> SQLSERVER TRUE patched 32.918 1.654
> (sameTable=true means that all threads were trying to allocate ids for the same db.table,
> false means they all targeted different tables)
>
>
> Thanks,
>
> Marton Bod
>
>
Re: Review Request 72481: HIVE-23234: Optimize
TxnHandler::allocateTableWriteIds
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/72481/#review220758
-----------------------------------------------------------
LGTM, some minor comments
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 2114 (original), 2105 (patched)
<https://reviews.apache.org/r/72481/#comment309437>
you can use txnToWriteIds.size() instead of counter
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
Line 4120 (original), 4114 (patched)
<https://reviews.apache.org/r/72481/#comment309439>
could we try not to place every method argument on a new line
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
Line 49 (original), 49 (patched)
<https://reviews.apache.org/r/72481/#comment309438>
Could you please. remove checkLock here as well.
- Denys Kuzmenko
On May 7, 2020, 3:55 p.m., Marton Bod wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72481/
> -----------------------------------------------------------
>
> (Updated May 7, 2020, 3:55 p.m.)
>
>
> Review request for hive, Denys Kuzmenko and Peter Vary.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Removed global mutex on writeId allocation, which means write ids can now be allocated concurrently for different tables without blocking each other, speeding up execution (perf test results below). Concurrent allocateTableWriteIds() operations targeting the same table are still mutexed by an S4U if the table is already present in next_write_id, otherwise a race condition to insert the table into next_write_id is solved by retrying after catching the duplicate key exception (the thread which commits later will be the one to retry).
>
> The situation is similar when allocateTableWriteIds() and replTableWriteIdState() are running concurrently - if they target different tables, they won't block each other anymore. If they target the same table, and the table is already inserted into next_write_id, replTableWriteIdState() returns early and allocateTableWriteIds() updates the next id. If the table is not yet in next_write_id, they might attempt to insert the same row concurrently, in which case who commits later will get a duplicate key exception and retry the operation, just as above.
>
>
> Diffs
> -----
>
> ql/src/test/org/apache/hadoop/hive/metastore/txn/TestTxnHandler.java 868da0c7a0
> 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/TxnHandler.java cf41ef8aaf
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java 1e177f4a7b
>
>
> Diff: https://reviews.apache.org/r/72481/diff/1/
>
>
> Testing
> -------
>
> Unit test in TestTxnHandler
> + Perf tests:
> dbType sameTable variant ms/op error
> MYSQL FALSE original 46.93 3.041
> MYSQL FALSE patched 19.283 1.311
> MYSQL TRUE original 50.185 3.595
> MYSQL TRUE patched 32.254 2.164
> ORACLE FALSE original 57.609 4.461
> ORACLE FALSE patched 25.721 2.551
> ORACLE TRUE original 59.668 3.172
> ORACLE TRUE patched 39.061 2.548
> POSTGRES FALSE original 39.364 2.94
> POSTGRES FALSE patched 18.518 1.038
> POSTGRES TRUE original 39.868 2.679
> POSTGRES TRUE patched 28.874 1.768
> SQLSERVER FALSE original 45.252 1.643
> SQLSERVER FALSE patched 24.583 1.529
> SQLSERVER TRUE original 49.149 3.45
> SQLSERVER TRUE patched 32.918 1.654
> (sameTable=true means that all threads were trying to allocate ids for the same db.table,
> false means they all targeted different tables)
>
>
> Thanks,
>
> Marton Bod
>
>