[iceberg] branch master updated: HIVE: Fix the Hive version checking in MetastoreLock logical error (#8547)

[pvary]

This is an automated email from the ASF dual-hosted git repository.

pvary pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iceberg.git


The following commit(s) were added to refs/heads/master by this push:
     new e10453a10d HIVE: Fix the Hive version checking in MetastoreLock 
logical error (#8547)
e10453a10d is described below

commit e10453a10d3f9237032ba7e649808347c699f6a2
Author: Paddy Gu <gcgpa...@163.com>
AuthorDate: Fri Sep 15 13:59:02 2023 +0800

    HIVE: Fix the Hive version checking in MetastoreLock logical error (#8547)
    
    Thanks @Paddy0523 for finding the bug and for the fix
---
 .../org/apache/iceberg/hive/MetastoreLock.java     |  2 +-
 .../apache/iceberg/hive/TestHiveCommitLocks.java   | 31 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git 
a/hive-metastore/src/main/java/org/apache/iceberg/hive/MetastoreLock.java 
b/hive-metastore/src/main/java/org/apache/iceberg/hive/MetastoreLock.java
index 5820365339..c77cfb896c 100644
--- a/hive-metastore/src/main/java/org/apache/iceberg/hive/MetastoreLock.java
+++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/MetastoreLock.java
@@ -310,7 +310,7 @@ class MetastoreLock implements HiveLock {
                 try {
                   // If we can not check for lock, or we do not find it, then 
rethrow the exception
                   // Otherwise we are happy as the findLock sets the lockId 
and the state correctly
-                  if (!HiveVersion.min(HiveVersion.HIVE_2)) {
+                  if (HiveVersion.min(HiveVersion.HIVE_2)) {
                     LockInfo lockFound = findLock();
                     if (lockFound != null) {
                       lockInfo.lockId = lockFound.lockId;
diff --git 
a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java 
b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java
index 9704b9f722..2d0f3d23ad 100644
--- 
a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java
+++ 
b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java
@@ -27,6 +27,8 @@ import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.spy;
@@ -66,6 +68,7 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.AdditionalAnswers;
 import org.mockito.ArgumentCaptor;
+import org.mockito.MockedStatic;
 import org.mockito.invocation.InvocationOnMock;
 
 public class TestHiveCommitLocks extends HiveTableBaseTest {
@@ -202,6 +205,7 @@ public class TestHiveCommitLocks extends HiveTableBaseTest {
 
     spyOps.doCommit(metadataV2, metadataV1);
 
+    verify(spyClient, times(1)).showLocks(any()); // Make sure HiveLock's 
findLock method is called
     assertThat(spyOps.current().schema().columns()).hasSize(1); // should be 1 
again
   }
 
@@ -226,6 +230,7 @@ public class TestHiveCommitLocks extends HiveTableBaseTest {
 
     spyOps.doCommit(metadataV2, metadataV1);
 
+    verify(spyClient, times(1)).showLocks(any()); // Make sure HiveLock's 
findLock method is called
     assertThat(spyOps.current().schema().columns()).hasSize(1); // should be 1 
again
   }
 
@@ -414,6 +419,32 @@ public class TestHiveCommitLocks extends HiveTableBaseTest 
{
             "org.apache.iceberg.hive.LockException: Metastore operation failed 
for hivedb.tbl");
   }
 
+  @Test
+  public void testPassThroughThriftExceptionsForHiveVersion_1()
+      throws TException, InterruptedException {
+    try (MockedStatic<HiveVersion> ignore = mockStatic(HiveVersion.class)) {
+      // default order is 0, meets the requirements of this test
+      HiveVersion version = mock(HiveVersion.class);
+      when(HiveVersion.current()).thenReturn(version);
+
+      doReturn(emptyLocks).when(spyClient).showLocks(any());
+      doThrow(new TException("Failed to connect to HMS"))
+          .doReturn(waitLockResponse)
+          .when(spyClient)
+          .lock(any());
+      doReturn(waitLockResponse)
+          .doReturn(acquiredLockResponse)
+          .when(spyClient)
+          .checkLock(eq(dummyLockId));
+      doNothing().when(spyClient).heartbeat(eq(0L), eq(dummyLockId));
+
+      assertThatThrownBy(() -> spyOps.doCommit(metadataV2, metadataV1))
+          .isInstanceOf(CommitFailedException.class)
+          .hasMessage(
+              "org.apache.iceberg.hive.LockException: Failed to find lock for 
table hivedb.tbl");
+    }
+  }
+
   @Test
   public void testPassThroughInterruptions() throws TException {
     doReturn(waitLockResponse).when(spyClient).lock(any());