You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/05/02 12:03:06 UTC
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3089: HIVE-26023: Non blocking REPLACE, RENAME COLUMNS implementation
veghlaci05 commented on code in PR #3089:
URL: https://github.com/apache/hive/pull/3089#discussion_r862778203
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3592,4 +3592,94 @@ public void testDropTableNonBlocking2() throws Exception {
driver.getFetchTask().fetch(res);
Assert.assertEquals("No records found", 2, res.size());
}
-}
+
+ @Test
+ public void testReplaceColumnsNonBlocking() throws Exception {
+ //TODO: incorporate add new column via `REPLACE` inside of `ADD COLUMNS` test case
+ testReplaceColumns(false);
+ }
+ @Test
+ public void testReplaceColumnsBlocking() throws Exception {
+ testReplaceColumns(true);
+ }
+ private void testReplaceColumns(boolean blocking) throws Exception {
+ testReplaceRenameColumns(blocking, "replace columns (c string, a bigint)");
+ }
+
+ @Test
+ public void testRenameColumnsNonBlocking() throws Exception {
+ testRenameColumns(false);
+ }
+ @Test
+ public void testRenameColumnsBlocking() throws Exception {
+ testRenameColumns(true);
+ }
+ private void testRenameColumns(boolean blocking) throws Exception {
+ testReplaceRenameColumns(blocking, "change column a c string");
+ }
+
+ private void testReplaceRenameColumns(boolean blocking, String alterSubQuery) throws Exception {
+ dropTable(new String[] {"tab_acid"});
+
+ HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_ACID_LOCKLESS_READS_ENABLED, !blocking);
+ driver = Mockito.spy(driver);
+
+ HiveConf.setBoolVar(driver2.getConf(), HiveConf.ConfVars.HIVE_ACID_LOCKLESS_READS_ENABLED, !blocking);
+ driver2 = Mockito.spy(driver2);
+
+ driver.run("create table if not exists tab_acid (a int, b int) " +
+ "stored as orc TBLPROPERTIES ('transactional'='true')");
+ driver.run("insert into tab_acid (a,b) values(1,2),(3,4)");
+
+ driver.compileAndRespond("select * from tab_acid");
+ List<String> res = new ArrayList<>();
+
+ driver.lockAndRespond();
+ List<ShowLocksResponseElement> locks = getLocks();
+ Assert.assertEquals("Unexpected lock count", 1, locks.size());
+
+ checkLock(LockType.SHARED_READ,
+ LockState.ACQUIRED, "default", "tab_acid", null, locks);
+
+ DbTxnManager txnMgr2 = (DbTxnManager) TxnManagerFactory.getTxnManagerFactory().getTxnManager(conf);
+ swapTxnManager(txnMgr2);
+ driver2.compileAndRespond("alter table tab_acid "+ alterSubQuery);
+
+ if (blocking) {
+ txnMgr2.acquireLocks(driver2.getPlan(), ctx, null, false);
+ locks = getLocks();
+
+ ShowLocksResponseElement checkLock = checkLock(LockType.EXCLUSIVE,
+ LockState.WAITING, "default", "tab_acid", null, locks);
+
+ swapTxnManager(txnMgr);
+ Mockito.doNothing().when(driver).lockAndRespond();
+ driver.run();
+
+ driver.getFetchTask().fetch(res);
+ swapTxnManager(txnMgr2);
+
+ FieldSetter.setField(txnMgr2, txnMgr2.getClass().getDeclaredField("numStatements"), 0);
Review Comment:
Why is this set needed?
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3592,4 +3592,94 @@ public void testDropTableNonBlocking2() throws Exception {
driver.getFetchTask().fetch(res);
Assert.assertEquals("No records found", 2, res.size());
}
-}
+
+ @Test
+ public void testReplaceColumnsNonBlocking() throws Exception {
+ //TODO: incorporate add new column via `REPLACE` inside of `ADD COLUMNS` test case
+ testReplaceColumns(false);
+ }
+ @Test
+ public void testReplaceColumnsBlocking() throws Exception {
+ testReplaceColumns(true);
+ }
+ private void testReplaceColumns(boolean blocking) throws Exception {
+ testReplaceRenameColumns(blocking, "replace columns (c string, a bigint)");
+ }
+
+ @Test
+ public void testRenameColumnsNonBlocking() throws Exception {
+ testRenameColumns(false);
+ }
+ @Test
+ public void testRenameColumnsBlocking() throws Exception {
+ testRenameColumns(true);
+ }
+ private void testRenameColumns(boolean blocking) throws Exception {
+ testReplaceRenameColumns(blocking, "change column a c string");
+ }
+
+ private void testReplaceRenameColumns(boolean blocking, String alterSubQuery) throws Exception {
+ dropTable(new String[] {"tab_acid"});
+
+ HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_ACID_LOCKLESS_READS_ENABLED, !blocking);
+ driver = Mockito.spy(driver);
+
+ HiveConf.setBoolVar(driver2.getConf(), HiveConf.ConfVars.HIVE_ACID_LOCKLESS_READS_ENABLED, !blocking);
+ driver2 = Mockito.spy(driver2);
+
+ driver.run("create table if not exists tab_acid (a int, b int) " +
+ "stored as orc TBLPROPERTIES ('transactional'='true')");
+ driver.run("insert into tab_acid (a,b) values(1,2),(3,4)");
+
+ driver.compileAndRespond("select * from tab_acid");
+ List<String> res = new ArrayList<>();
+
+ driver.lockAndRespond();
+ List<ShowLocksResponseElement> locks = getLocks();
+ Assert.assertEquals("Unexpected lock count", 1, locks.size());
+
+ checkLock(LockType.SHARED_READ,
+ LockState.ACQUIRED, "default", "tab_acid", null, locks);
+
+ DbTxnManager txnMgr2 = (DbTxnManager) TxnManagerFactory.getTxnManagerFactory().getTxnManager(conf);
+ swapTxnManager(txnMgr2);
+ driver2.compileAndRespond("alter table tab_acid "+ alterSubQuery);
+
+ if (blocking) {
+ txnMgr2.acquireLocks(driver2.getPlan(), ctx, null, false);
+ locks = getLocks();
+
+ ShowLocksResponseElement checkLock = checkLock(LockType.EXCLUSIVE,
+ LockState.WAITING, "default", "tab_acid", null, locks);
+
+ swapTxnManager(txnMgr);
+ Mockito.doNothing().when(driver).lockAndRespond();
+ driver.run();
+
+ driver.getFetchTask().fetch(res);
+ swapTxnManager(txnMgr2);
+
+ FieldSetter.setField(txnMgr2, txnMgr2.getClass().getDeclaredField("numStatements"), 0);
+ txnMgr2.getMS().unlock(checkLock.getLockid());
+ }
+ driver2.lockAndRespond();
+ locks = getLocks();
+ Assert.assertEquals("Unexpected lock count", blocking ? 1 : 2, locks.size());
+
+ checkLock(blocking ? LockType.EXCLUSIVE : LockType.EXCL_WRITE,
+ LockState.ACQUIRED, "default", "tab_acid", null, locks);
Review Comment:
This could be changed to sth like
` checkLock(blocking ? LockType.EXCLUSIVE : LockType.EXCL_WRITE,
blocking ? LockState.WAITING : LockState.ACQUIRED, "default", "tab_acid", null, locks);`
In this case the code block inside `if (blocking)` would not be necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org