> On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > LGMT, just few questions
Marton, thank you for the review! > On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > > Lines 2553 (patched) > > <https://reviews.apache.org/r/72436/diff/1/?file=2227947#file2227947line2553> > > > > nit: white space plus explanation mentions "wrong code" but it's the > > whole ErrorMsg that's being compared fixed, regarding the ex.getCanonicalErrorMsg() - it's referenced as a code in every test method. Probably confusing but at least consistent. > On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > > Lines 2555 (patched) > > <https://reviews.apache.org/r/72436/diff/1/?file=2227947#file2227947line2555> > > > > perhaps move this errorMsg into a constant since it's being referenced > > more than once (or into ErrorMsg) used ErrorMsg.format > On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > > Line 2559 (original), 2588 (patched) > > <https://reviews.apache.org/r/72436/diff/1/?file=2227947#file2227947line2588> > > > > it might make sense to extract this setup/query running part into a > > common method, and put the different assertions into testFairness and > > testFairnessZeroWaitRead, that might make the assertion part easier to > > follow. Same above these tests have absolutely the same setups and exec queries - just different outcome. Most of existing tests follow the same approach. I would keep it as is for now, maybe we'll have to refactor whole stuff later. > On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/LockRequest.java > > Lines 59 (patched) > > <https://reviews.apache.org/r/72436/diff/1/?file=2227948#file2227948line59> > > > > quick question: does this need to be part of the API, or can we just > > derive it from the conf vars (TXN_WRITE_X_LOCK, TXN_OVERWRITE_X_LOCK) as we > > do in the tests? TXN_WRITE_X_LOCK, TXN_OVERWRITE_X_LOCK are both HS2 configs that are not propagated to HMS + they have to be session level configs. I am not sure if there is a better way to do this. > On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Lines 4389 (patched) > > <https://reviews.apache.org/r/72436/diff/1/?file=2227954#file2227954line4392> > > > > do we need this delete? if the txn is aborted, wouldn't all the locks > > be removed in abortTxn? if we won't delete them here, locks would be cleaned only once housekeeper thread starts, not earlier. > On April 27, 2020, 10:03 a.m., Marton Bod wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > > Lines 4391 (patched) > > <https://reviews.apache.org/r/72436/diff/1/?file=2227954#file2227954line4394> > > > > shouldn't we check blockedBy for certainty that it was an exclusive > > lock that blocked us? (and log the anomaly if not) this condition is baked into the check conflict query - Denys ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72436/#review220497 ----------------------------------------------------------- On April 27, 2020, 11:24 a.m., Denys Kuzmenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72436/ > ----------------------------------------------------------- > > (Updated April 27, 2020, 11:24 a.m.) > > > Review request for hive, Marton Bod and Peter Vary. > > > Bugs: HIVE-23293 > https://issues.apache.org/jira/browse/HIVE-23293 > > > Repository: hive-git > > > Description > ------- > > With a new lock type (EXCL_WRITE) for INSERT_OVERWRITE, SHARED_READ does not > have to wait for any lock - it can fails fast for a pending EXCLUSIVE, > because even if there is an EXCL_WRITE or SHARED_WRITE pending, there's no > semantic reason to wait for them. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java b4dac4346e > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > 497cedd61f > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/LockRequest.java > 7402fb30eb > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php > 9fb7ff011a > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ttypes.py > 4f317b3453 > > standalone-metastore/metastore-common/src/gen/thrift/gen-rb/hive_metastore_types.rb > e64ae0ead2 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/LockRequestBuilder.java > 93da0f60ec > standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift > 1e3d6e9b8b > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > 580786832e > > > Diff: https://reviews.apache.org/r/72436/diff/2/ > > > Testing > ------- > > DbTxnManager tests > > > Thanks, > > Denys Kuzmenko > >
