> On May 4, 2020, 3:21 p.m., Marton Bod wrote: > > Looks good to me, just a couple of questions.
Thank you for the review! > On May 4, 2020, 3:21 p.m., Marton Bod wrote: > > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > > Lines 2226 (patched) > > <https://reviews.apache.org/r/72470/diff/1/?file=2230136#file2230136line2226> > > > > nit: the .getLockid() part is a leftover? removed. > On May 4, 2020, 3:21 p.m., Marton Bod wrote: > > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > > Lines 2241 (patched) > > <https://reviews.apache.org/r/72470/diff/1/?file=2230136#file2230136line2241> > > > > maybe worth running the same tests with 'false' too? added. - Denys ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72470/#review220595 ----------------------------------------------------------- On May 4, 2020, 4:33 p.m., Denys Kuzmenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72470/ > ----------------------------------------------------------- > > (Updated May 4, 2020, 4:33 p.m.) > > > Review request for hive, Marton Bod, Peter Varga, and Peter Vary. > > > Bugs: HIVE-23349 > https://issues.apache.org/jira/browse/HIVE-23349 > > > Repository: hive-git > > > Description > ------- > > 2 concurrent MERGE INSERT operations generate duplicates due to lack of > locking. > MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that > doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or > EXCL_WRITE if hive.txn.write.xlock=false; > > create table target (a int, b int) stored as orc TBLPROPERTIES > ('transactional'='true')"); > insert into target values (1,2), (3,4) > create table source (a int, b int) > execute in parallel: > > insert into source values (5,6), (7,8) > > PS: > > Current patch doesn cover following scenario: > 1) T1 (merge-insert) opens txns & records snapshot; > 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it; > 3) T2 commits it's changes and unlocks T1; > 4) T1 locks snapshot and validates txn list, however only txns with txnId > lower than T1's is beeing considered, T2 changes are ignored -> duplicates > are generated. > > > merge-insert/merge-insert scenario could be fixed by leveraging write-write > conflict check mechanism. We just need to set operation type for merge-insert > to update. > However it won't solve issue with merge-insert/insert. > > We should consider moving locking before snapshot generation, current > snapshot re-check mechanism doesn't handle described scenarios. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 > ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f > ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java > 3ffdcec528 > > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java > b435e79c3c > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > 1687425bcb > ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd > > > Diff: https://reviews.apache.org/r/72470/diff/2/ > > > Testing > ------- > > Manual, added number of merge related test scenarios into TestDbTxnManager2, > modified explain_locks.q > > > Thanks, > > Denys Kuzmenko > >
