You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by es...@apache.org on 2019/02/13 19:14:42 UTC
[geode] branch feature/GEODE-6379 updated: GEODE-6379: fix review
comments.
This is an automated email from the ASF dual-hosted git repository.
eshu11 pushed a commit to branch feature/GEODE-6379
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/feature/GEODE-6379 by this push:
new dbfc7e2 GEODE-6379: fix review comments.
dbfc7e2 is described below
commit dbfc7e28c566c2cb7bb002540e8879a943320c6c
Author: eshu <es...@pivotal.io>
AuthorDate: Wed Feb 13 11:14:00 2019 -0800
GEODE-6379: fix review comments.
---
.../distributed/internal/locks/DLockGrantor.java | 54 +++++++++++++---------
.../internal/locks/DLockGrantorTest.java | 10 ++--
2 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
index 1b848d6..6157e59 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockGrantor.java
@@ -15,12 +15,15 @@
package org.apache.geode.distributed.internal.locks;
+import static java.util.concurrent.TimeUnit.DAYS;
+
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
+import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@@ -148,9 +151,8 @@ public class DLockGrantor {
*/
private final TXReservationMgr resMgr = new TXReservationMgr(false);
- private final Map<InternalDistributedMember, Long> membersDepartedTime = new HashMap();
- private final int membersDepartedTimeKeptSize = 10;
- private final long departedMemberKeptInMapMilliSeconds = 24 * 60 * 60 * 1000;
+ private final Map<InternalDistributedMember, Long> membersDepartedTime = new LinkedHashMap();
+ private final long departedMemberKeptInMapMilliSeconds = DAYS.toMillis(1);
/**
* Enforces waiting until this grantor is initialized. Used to block all lock requests until
@@ -504,13 +506,8 @@ public class DLockGrantor {
}
DLockBatch batch = (DLockBatch) request.getObjectName();
- synchronized (membersDepartedTime) {
- // the transaction host/txLock requester has departed.
- if (membersDepartedTime.containsKey(batch.getOwner())) {
- throw new TransactionDataNodeHasDepartedException("Transaction host has been departed");
- }
- resMgr.makeReservation((IdentityArrayList) batch.getReqs());
- }
+ checkIfHostDeparted(batch.getOwner());
+ resMgr.makeReservation((IdentityArrayList) batch.getReqs());
if (isTraceEnabled_DLS) {
logger.trace(LogMarker.DLS_VERBOSE, "[DLockGrantor.handleLockBatch] granting {}",
batch.getBatchId());
@@ -525,6 +522,17 @@ public class DLockGrantor {
}
}
+ private void checkIfHostDeparted(InternalDistributedMember owner) {
+ // Already held batchLocks; hold membersDepartedTime lock just for clarity
+ synchronized (membersDepartedTime) {
+ // the transaction host/txLock requester has departed.
+ if (membersDepartedTime.containsKey(owner)) {
+ throw new TransactionDataNodeHasDepartedException(
+ "The transaction host " + owner + " is no longer a member of the cluster.");
+ }
+ }
+ }
+
/**
* Returns transaction optimized lock batches that were created by the specified owner.
* <p>
@@ -534,12 +542,12 @@ public class DLockGrantor {
* @return lock batches that were created by owner
*/
public DLockBatch[] getLockBatches(InternalDistributedMember owner) {
- // put owner into the map first so that no new threads will handle in-flight requests
- // from the departed member to lock keys
- recordMemberDepartedTime(owner);
-
// Key: Object batchId, Value: DLockBatch batch
synchronized (this.batchLocks) {
+ // put owner into the map first so that no new threads will handle in-flight requests
+ // from the departed member to lock keys
+ recordMemberDepartedTime(owner);
+
List batchList = new ArrayList();
for (Iterator iter = this.batchLocks.values().iterator(); iter.hasNext();) {
DLockBatch batch = (DLockBatch) iter.next();
@@ -552,18 +560,22 @@ public class DLockGrantor {
}
void recordMemberDepartedTime(InternalDistributedMember owner) {
+ // Already held batchLocks; hold membersDepartedTime lock just for clarity
synchronized (membersDepartedTime) {
- if (membersDepartedTime.size() >= membersDepartedTimeKeptSize) {
- // remove all departed members kept in map longer than 1 day
- membersDepartedTime.entrySet()
- .removeIf(entries -> entries.getValue() < departedMembersToBeExpiredTime());
+ long currentTime = getCurrentTime();
+ for (Iterator iterator = membersDepartedTime.values().iterator(); iterator.hasNext();) {
+ if ((long) iterator.next() < currentTime - departedMemberKeptInMapMilliSeconds) {
+ iterator.remove();
+ } else {
+ break;
+ }
}
- membersDepartedTime.put(owner, System.currentTimeMillis());
+ membersDepartedTime.put(owner, currentTime);
}
}
- long departedMembersToBeExpiredTime() {
- return System.currentTimeMillis() - departedMemberKeptInMapMilliSeconds;
+ long getCurrentTime() {
+ return System.currentTimeMillis();
}
@VisibleForTesting
diff --git a/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockGrantorTest.java b/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockGrantorTest.java
index 5b0fa23..99d3c02 100644
--- a/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockGrantorTest.java
+++ b/geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockGrantorTest.java
@@ -14,6 +14,7 @@
*/
package org.apache.geode.distributed.internal.locks;
+import static java.util.concurrent.TimeUnit.DAYS;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
@@ -74,12 +75,15 @@ public class DLockGrantorTest {
@Test
public void recordMemberDepartedTimeRemovesExpiredMembers() {
DLockGrantor spy = spy(grantor);
- for (int i = 0; i < 10; i++) {
+ long currentTime = System.currentTimeMillis();
+ doReturn(currentTime).doReturn(currentTime).doReturn(currentTime + 1 + DAYS.toMillis(1))
+ .when(spy).getCurrentTime();
+
+ for (int i = 0; i < 2; i++) {
spy.recordMemberDepartedTime(mock(InternalDistributedMember.class));
}
- assertThat(spy.getMembersDepartedTimeRecords().size()).isEqualTo(10);
+ assertThat(spy.getMembersDepartedTimeRecords().size()).isEqualTo(2);
- doReturn(System.currentTimeMillis() + 1).when(spy).departedMembersToBeExpiredTime();
InternalDistributedMember owner = mock(InternalDistributedMember.class);
spy.recordMemberDepartedTime(owner);