DonalEvans commented on a change in pull request #6470:
URL: https://github.com/apache/geode/pull/6470#discussion_r631444933
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
##########
@@ -663,7 +675,8 @@ public void updateLockBatch(Object batchId, DLockBatch
newBatch) throws Interrup
/**
* Releases the transaction optimized lock batch.
* <p>
- * Acquires acquireDestroyReadLock. Synchronizes on batchLocks.
+ * Acquired destroy read lock before synchronizes on {@link #batchLocks}.
Review comment:
I think this should be "Acquires destroy read lock before synchronizing
on"
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
##########
@@ -472,7 +472,8 @@ private void throwIfDestroyed(boolean destroyed) {
/**
* Handles request for a batch of locks using optimization for transactions.
* <p>
- * Synchronizes on {@link #batchLocks}.
+ * Acquired destroy read lock before synchronizes on {@link #batchLocks}.
Review comment:
I think this should be "Acquires destroy read lock before synchronizing
on"
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
##########
@@ -625,7 +634,8 @@ public DLockBatch getLockBatch(Object batchId) throws
InterruptedException {
* Update the batch for the given batch. This operation was added as part of
the solution to bug
* 32999.
* <p>
- * Acquires acquireDestroyReadLock. Synchronizes on batchLocks.
+ * Acquired destroy read lock before synchronizes on {@link #batchLocks}.
Review comment:
I think this should be "Acquires destroy read lock before synchronizing
on"
##########
File path:
geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockGrantorTest.java
##########
@@ -18,28 +18,40 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import org.junit.Before;
import org.junit.Test;
import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.CommitConflictException;
import org.apache.geode.cache.TransactionDataNodeHasDepartedException;
import org.apache.geode.distributed.internal.DistributionManager;
import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
public class DLockGrantorTest {
private DLockService dLockService;
private DLockGrantor grantor;
+ private final DLockBatchId batchId = mock(DLockBatchId.class);
+ private final DLockBatch batch = mock(DLockBatch.class);
+ private final DLockRequestProcessor.DLockRequestMessage request = mock(
+ DLockRequestProcessor.DLockRequestMessage.class);
+ private final InternalDistributedMember owner =
mock(InternalDistributedMember.class);
@Before
public void setup() {
dLockService = mock(DLockService.class, RETURNS_DEEP_STUBS);
DistributionManager distributionManager = mock(DistributionManager.class);
when(dLockService.getDistributionManager()).thenReturn(distributionManager);
+ when(dLockService.getDLockLessorDepartureHandler())
Review comment:
This is addition not necessary, as `dLockService` was created using
`RETURNS_DEEP_STUBS`, so calling any method on it will return a mocked class.
In fact, none of the mocking in this `setup()` method is necessary other than:
```
dLockService = mock(DLockService.class, RETURNS_DEEP_STUBS);
grantor = DLockGrantor.createGrantor(dLockService, 1);
```
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
##########
@@ -587,7 +593,8 @@ long getCurrentTime() {
* Get the batch for the given batchId (for example use a txLockId from
TXLockBatch in order to
* update its participants). This operation was added as part of the
solution to bug 32999.
* <p>
- * Acquires acquireDestroyReadLock. Synchronizes on batchLocks.
+ * Acquired destroy read lock before synchronizes on {@link #batchLocks}.
Review comment:
I think this should be "Acquires destroy read lock before synchronizing
on"
##########
File path:
geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockGrantorTest.java
##########
@@ -83,11 +94,175 @@ public void
recordMemberDepartedTimeRemovesExpiredMembers() {
}
assertThat(spy.getMembersDepartedTimeRecords().size()).isEqualTo(2);
- InternalDistributedMember owner = mock(InternalDistributedMember.class);
spy.recordMemberDepartedTime(owner);
assertThat(spy.getMembersDepartedTimeRecords().size()).isEqualTo(1);
assertThat(spy.getMembersDepartedTimeRecords()).containsKey(owner);
}
+ @Test
+ public void cleanupSuspendStateIfRequestHasTimedOut() throws Exception {
+ DLockGrantor spy = spy(grantor);
Review comment:
This can be moved to the `setup()` method as
```
grantor = spy(DLockGrantor.createGrantor(dLockService, 1));
```
since almost all the tests use a spy on `grantor`. Doing this means you
don't need to create a new `spy` variable in each test, but can just directly
use `grantor`.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
