From 8664367d5e020036c0d2d7858ff138dc83efaa3d Mon Sep 17 00:00:00 2001 From: YxAc Date: Thu, 8 Feb 2024 01:00:20 +0800 Subject: [PATCH 1/5] improve TreeLock for ease of unlock and fix unexpected err when lock --- .../datastrato/gravitino/lock/TreeLock.java | 42 ++++++++------- .../gravitino/lock/TestTreeLock.java | 51 +++++++++++++++---- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java index 71e23dbdb36..7ed98dd5319 100644 --- a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java +++ b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java @@ -6,8 +6,12 @@ package com.datastrato.gravitino.lock; import com.datastrato.gravitino.NameIdentifier; + +import java.util.Deque; import java.util.List; -import java.util.Stack; +import java.util.concurrent.ConcurrentLinkedDeque; + +import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -62,8 +66,8 @@ public class TreeLock { // TreeLockNode to be locked private final List lockNodes; - // TreeLockNode that has been locked. - private final Stack heldLocks = new Stack<>(); + // TreeLockNode that has been locked along with its lock type. + private final Deque> heldLocks = new ConcurrentLinkedDeque<>(); private LockType lockType; TreeLock(List lockNodes, NameIdentifier identifier) { @@ -73,6 +77,8 @@ public class TreeLock { /** * Lock the tree lock with the given lock type. + * This method locks all nodes in the list, from the root to the leaf, and pushes them onto the deque. + * If an exception occurs during the locking process, it will unlock all nodes that have been locked so far. * * @param lockType The lock type to lock the tree lock. */ @@ -83,8 +89,16 @@ public void lock(LockType lockType) { for (int i = 0; i < length; i++) { TreeLockNode treeLockNode = lockNodes.get(i); LockType type = i == length - 1 ? lockType : LockType.READ; - treeLockNode.lock(type); - heldLocks.push(treeLockNode); + try { + treeLockNode.lock(type); + heldLocks.push(Pair.of(treeLockNode, type)); + LOG.trace("Locked node: {}, lock type: {}", treeLockNode, type); + } catch (Exception e) { + LOG.error("Failed to lock the treeNode, identifier: {}, node {} of lockNodes: [{}]", + identifier, treeLockNode, lockNodes, e); + unlock(); + throw e; + } } if (LOG.isTraceEnabled()) { @@ -102,25 +116,17 @@ public void unlock() { throw new IllegalStateException("We must lock the tree lock before unlock it."); } - boolean lastNode = false; - TreeLockNode current; while (!heldLocks.isEmpty()) { - LockType type; - if (!lastNode) { - lastNode = true; - type = lockType; - } else { - type = LockType.READ; - } - - current = heldLocks.pop(); - // Unlock the node and decrease the reference count. + Pair pair = heldLocks.pop(); + TreeLockNode current = pair.getLeft(); + LockType type = pair.getRight(); current.unlock(type); + LOG.trace("Unlocked node: {}, lock type: {}", current, type); } if (LOG.isTraceEnabled()) { LOG.trace( - "Unlocked the tree lock, ident: {}, lockNodes: [{}], lock type: {}", + "Unlocked the tree lock, identifier: {}, lockNodes: [{}], lock type: {}", identifier, lockNodes, lockType); diff --git a/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java b/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java index 235afd3e859..66b855427f3 100644 --- a/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java +++ b/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java @@ -6,23 +6,52 @@ package com.datastrato.gravitino.lock; import static com.datastrato.gravitino.lock.TestLockManager.getConfig; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; class TestTreeLock { + private LockManager lockManager; + private TreeLock lock; + + @BeforeEach + void setUp() { + lockManager = new LockManager(getConfig()); + lock = lockManager.createTreeLock(TestLockManager.randomNameIdentifier()); + } + + @Test + void testLockAndUnlockWithReadLock() { + assertDoesNotThrow(() -> { + lock.lock(LockType.READ); + lock.unlock(); + }); + } + + @Test + void testLockAndUnlockWithWriteLock() { + assertDoesNotThrow(() -> { + lock.lock(LockType.WRITE); + lock.unlock(); + }); + } + + @Test + void testUnlockWithoutLock() { + assertThrows(IllegalStateException.class, () -> { + lock.unlock(); + }); + } @Test - void testLock() { - LockManager lockManager = new LockManager(getConfig()); - - for (int i = 0; i < 1000; i++) { - TreeLock lock = lockManager.createTreeLock(TestLockManager.randomNameIdentifier()); - lock.lock(i % 2 == 0 ? LockType.READ : LockType.WRITE); - try { - // Business logic here. - } finally { + void testMultipleLockAndUnlock() { + assertDoesNotThrow(() -> { + for (int i = 0; i < 1000; i++) { + lock.lock(i % 2 == 0 ? LockType.READ : LockType.WRITE); lock.unlock(); } - } + }); } -} +} \ No newline at end of file From 3a9398e335b471847f02fb81b1e46fc13cf08e89 Mon Sep 17 00:00:00 2001 From: YxAc Date: Thu, 8 Feb 2024 01:12:06 +0800 Subject: [PATCH 2/5] fix format --- .../datastrato/gravitino/lock/TreeLock.java | 16 ++++---- .../gravitino/lock/TestTreeLock.java | 41 +++++++++++-------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java index 7ed98dd5319..6d17ae8230c 100644 --- a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java +++ b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java @@ -6,11 +6,9 @@ package com.datastrato.gravitino.lock; import com.datastrato.gravitino.NameIdentifier; - import java.util.Deque; import java.util.List; import java.util.concurrent.ConcurrentLinkedDeque; - import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -76,9 +74,9 @@ public class TreeLock { } /** - * Lock the tree lock with the given lock type. - * This method locks all nodes in the list, from the root to the leaf, and pushes them onto the deque. - * If an exception occurs during the locking process, it will unlock all nodes that have been locked so far. + * Lock the tree lock with the given lock type. This method locks all nodes in the list, from the + * root to the leaf, and pushes them onto the deque. If an exception occurs during the locking + * process, it will unlock all nodes that have been locked so far. * * @param lockType The lock type to lock the tree lock. */ @@ -94,8 +92,12 @@ public void lock(LockType lockType) { heldLocks.push(Pair.of(treeLockNode, type)); LOG.trace("Locked node: {}, lock type: {}", treeLockNode, type); } catch (Exception e) { - LOG.error("Failed to lock the treeNode, identifier: {}, node {} of lockNodes: [{}]", - identifier, treeLockNode, lockNodes, e); + LOG.error( + "Failed to lock the treeNode, identifier: {}, node {} of lockNodes: [{}]", + identifier, + treeLockNode, + lockNodes, + e); unlock(); throw e; } diff --git a/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java b/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java index 66b855427f3..217b22cca7a 100644 --- a/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java +++ b/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java @@ -24,34 +24,39 @@ void setUp() { @Test void testLockAndUnlockWithReadLock() { - assertDoesNotThrow(() -> { - lock.lock(LockType.READ); - lock.unlock(); - }); + assertDoesNotThrow( + () -> { + lock.lock(LockType.READ); + lock.unlock(); + }); } @Test void testLockAndUnlockWithWriteLock() { - assertDoesNotThrow(() -> { - lock.lock(LockType.WRITE); - lock.unlock(); - }); + assertDoesNotThrow( + () -> { + lock.lock(LockType.WRITE); + lock.unlock(); + }); } @Test void testUnlockWithoutLock() { - assertThrows(IllegalStateException.class, () -> { - lock.unlock(); - }); + assertThrows( + IllegalStateException.class, + () -> { + lock.unlock(); + }); } @Test void testMultipleLockAndUnlock() { - assertDoesNotThrow(() -> { - for (int i = 0; i < 1000; i++) { - lock.lock(i % 2 == 0 ? LockType.READ : LockType.WRITE); - lock.unlock(); - } - }); + assertDoesNotThrow( + () -> { + for (int i = 0; i < 1000; i++) { + lock.lock(i % 2 == 0 ? LockType.READ : LockType.WRITE); + lock.unlock(); + } + }); } -} \ No newline at end of file +} From 0236006f32ed1fb8cb6d4a74a8a8a2b99e78359b Mon Sep 17 00:00:00 2001 From: YxAc Date: Thu, 8 Feb 2024 15:03:00 +0800 Subject: [PATCH 3/5] add testLockFailureAndUnlock --- .../datastrato/gravitino/lock/TreeLock.java | 1 + .../gravitino/lock/TestTreeLock.java | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java index 6d17ae8230c..87522727ee4 100644 --- a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java +++ b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java @@ -98,6 +98,7 @@ public void lock(LockType lockType) { treeLockNode, lockNodes, e); + // unlock all nodes that have been locked when an exception occurs. unlock(); throw e; } diff --git a/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java b/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java index 217b22cca7a..3fcbb5bcebc 100644 --- a/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java +++ b/core/src/test/java/com/datastrato/gravitino/lock/TestTreeLock.java @@ -8,9 +8,13 @@ import static com.datastrato.gravitino.lock.TestLockManager.getConfig; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.doThrow; +import java.util.Arrays; +import java.util.List; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; class TestTreeLock { private LockManager lockManager; @@ -59,4 +63,29 @@ void testMultipleLockAndUnlock() { } }); } + + @Test + void testLockFailureAndUnlock() { + TreeLockNode mockNode1 = Mockito.mock(TreeLockNode.class); + TreeLockNode mockNode2 = Mockito.mock(TreeLockNode.class); + TreeLockNode mockNode3 = Mockito.mock(TreeLockNode.class); + + // Mock the lock method of the second node to throw an exception + doThrow(new RuntimeException("Mock exception")).when(mockNode2).lock(Mockito.any()); + + List lockNodes = Arrays.asList(mockNode1, mockNode2, mockNode3); + TreeLock treeLock = new TreeLock(lockNodes, TestLockManager.randomNameIdentifier()); + + assertThrows( + RuntimeException.class, + () -> treeLock.lock(LockType.WRITE), + "Expected lock to throw, but it didn't"); + + // Verify that the first node was unlocked + Mockito.verify(mockNode1, Mockito.times(1)).unlock(Mockito.any()); + + // Verify that the second and third nodes were not unlocked + Mockito.verify(mockNode2, Mockito.never()).unlock(Mockito.any()); + Mockito.verify(mockNode3, Mockito.never()).unlock(Mockito.any()); + } } From 40a61b942d115fc070533c269fcac9aa63052f15 Mon Sep 17 00:00:00 2001 From: YongXing Date: Sun, 18 Feb 2024 11:23:39 +0800 Subject: [PATCH 4/5] Update TreeLock.java add trace log judge --- .../src/main/java/com/datastrato/gravitino/lock/TreeLock.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java index 87522727ee4..3bce732225a 100644 --- a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java +++ b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java @@ -90,7 +90,9 @@ public void lock(LockType lockType) { try { treeLockNode.lock(type); heldLocks.push(Pair.of(treeLockNode, type)); - LOG.trace("Locked node: {}, lock type: {}", treeLockNode, type); + if (LOG.isTraceEnabled()) { + LOG.trace("Locked node: {}, lock type: {}", treeLockNode, type); + } } catch (Exception e) { LOG.error( "Failed to lock the treeNode, identifier: {}, node {} of lockNodes: [{}]", From 2bd83f1dddb0369221c34bb921da23db5dd53d1a Mon Sep 17 00:00:00 2001 From: YongXing Date: Sun, 18 Feb 2024 11:25:52 +0800 Subject: [PATCH 5/5] Update TreeLock.java add trace log judgement --- .../src/main/java/com/datastrato/gravitino/lock/TreeLock.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java index 3bce732225a..ec8076ff752 100644 --- a/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java +++ b/core/src/main/java/com/datastrato/gravitino/lock/TreeLock.java @@ -126,7 +126,9 @@ public void unlock() { TreeLockNode current = pair.getLeft(); LockType type = pair.getRight(); current.unlock(type); - LOG.trace("Unlocked node: {}, lock type: {}", current, type); + if (LOG.isTraceEnabled()) { + LOG.trace("Unlocked node: {}, lock type: {}", current, type); + } } if (LOG.isTraceEnabled()) {