You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/09/09 21:52:12 UTC
[GitHub] [geode] ringles opened a new pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
ringles opened a new pull request #5501:
URL: https://github.com/apache/geode/pull/5501
The Redis RENAME command could hang during a rebalance, if the old key was stored in a bucket on one server, and the new key was in a different bucket on a separate server. Rename would read-lock the buckets, but rebalance would wait for a write lock, and the Rename region put/destroy would then wait on the write lock.
Now Rename passes a callback argument that indicates it has already locked the primary, and will not attempt to lock the primary again when doing the put/destroy.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486491077
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -622,7 +623,9 @@ public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucke
}
try {
- br.doLockForPrimary(false);
+ if (!br.doLockForPrimary(false)) {
Review comment:
remove the "if" and just call br.doLockForPrimary(false);
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -611,7 +611,8 @@ PartitionedRegionRedundancyTracker getRedundancyTracker() {
return redundancyTracker;
}
- public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucketLockException {
+ public boolean computeWithPrimaryLocked(Object key, Runnable r)
Review comment:
this should no longer return boolean (since we got rid of the tryLock option)
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PrimaryBucketLockException.java
##########
@@ -19,7 +19,7 @@
import org.apache.geode.GemFireException;
/**
- * Thrown by {@link PartitionedRegion#computeWithPrimaryLocked(Object, Runnable)}
+ * Thrown by {@link PartitionedRegion#computeWithPrimaryLocked(Object, Runnable, boolean)}
Review comment:
this javadoc needs to be updated for the signature with no boolean
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -3395,11 +3399,10 @@ private InternalDistributedMember getNodeForBucketReadOrLoad(int bucketId) {
* @throws PrimaryBucketException if the remote bucket was not the primary
* @throws ForceReattemptException if the peer is no longer available
*/
+
public boolean putRemotely(final DistributedMember recipient, final EntryEventImpl event,
boolean ifNew, boolean ifOld, Object expectedOldValue, boolean requireOldValue)
throws PrimaryBucketException, ForceReattemptException {
- // boolean forceAck = basicGetWriter() != null
- // || getDistributionAdvisor().adviseNetWrite().size() > 0;
Review comment:
restore this comment; basically get this PR to the point that it does not modify PartitionedRegion.java
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -5718,7 +5718,6 @@ RegionEntry basicPutEntry(final EntryEventImpl event, final long lastModified)
@Override
public long basicPutPart2(EntryEventImpl event, RegionEntry entry, boolean isInitialized,
long lastModified, boolean clearConflict) {
-
Review comment:
put this blank line back so that this PR does not modify LocalRegion.java
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -3395,11 +3399,10 @@ private InternalDistributedMember getNodeForBucketReadOrLoad(int bucketId) {
* @throws PrimaryBucketException if the remote bucket was not the primary
* @throws ForceReattemptException if the peer is no longer available
*/
+
Review comment:
take out this new blank line
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486504966
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PrimaryBucketLockException.java
##########
@@ -19,7 +19,7 @@
import org.apache.geode.GemFireException;
/**
- * Thrown by {@link PartitionedRegion#computeWithPrimaryLocked(Object, Runnable)}
+ * Thrown by {@link PartitionedRegion#computeWithPrimaryLocked(Object, Runnable, boolean)}
Review comment:
Truth. Thanks, Darrel!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486504559
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -611,7 +611,8 @@ PartitionedRegionRedundancyTracker getRedundancyTracker() {
return redundancyTracker;
}
- public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucketLockException {
+ public boolean computeWithPrimaryLocked(Object key, Runnable r)
Review comment:
Good call, just updated it!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486505201
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -5718,7 +5718,6 @@ RegionEntry basicPutEntry(final EntryEventImpl event, final long lastModified)
@Override
public long basicPutPart2(EntryEventImpl event, RegionEntry entry, boolean isInitialized,
long lastModified, boolean clearConflict) {
-
Review comment:
updated.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486450726
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -611,7 +611,8 @@ PartitionedRegionRedundancyTracker getRedundancyTracker() {
return redundancyTracker;
}
- public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucketLockException {
+ public boolean computeWithPrimaryLocked(Object key, Runnable r, boolean tryLock)
Review comment:
My understanding is that we no longer use this new "tryLock" parameter. So should we remove it from this method so that this PR does not change this code?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/RedisKeyCommandsFunctionInvoker.java
##########
@@ -74,18 +73,18 @@ public String type(ByteArrayWrapper key) {
@Override
- @SuppressWarnings("unchecked")
public boolean rename(ByteArrayWrapper oldKey, ByteArrayWrapper newKey) {
+ while (true) {
Review comment:
I think the "while (true)" loop should be removed. I think it is a remnant of the tryLock code. As it is now I don't see that it would ever loop.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #5501:
URL: https://github.com/apache/geode/pull/5501
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486466651
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/RedisKeyCommandsFunctionInvoker.java
##########
@@ -74,18 +73,18 @@ public String type(ByteArrayWrapper key) {
@Override
- @SuppressWarnings("unchecked")
public boolean rename(ByteArrayWrapper oldKey, ByteArrayWrapper newKey) {
+ while (true) {
Review comment:
Truth, Darrel. Just removed it!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486463800
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -611,7 +611,8 @@ PartitionedRegionRedundancyTracker getRedundancyTracker() {
return redundancyTracker;
}
- public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucketLockException {
+ public boolean computeWithPrimaryLocked(Object key, Runnable r, boolean tryLock)
Review comment:
You are correct, Darrel, we no longer need it. Jens and I talked about it a bit this. morning and thought it might be beneficial to have the option in the method, but since it's not needed for this PR it probably makes sense to remove it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486505050
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -3395,11 +3399,10 @@ private InternalDistributedMember getNodeForBucketReadOrLoad(int bucketId) {
* @throws PrimaryBucketException if the remote bucket was not the primary
* @throws ForceReattemptException if the peer is no longer available
*/
+
Review comment:
updated.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -3395,11 +3399,10 @@ private InternalDistributedMember getNodeForBucketReadOrLoad(int bucketId) {
* @throws PrimaryBucketException if the remote bucket was not the primary
* @throws ForceReattemptException if the peer is no longer available
*/
+
public boolean putRemotely(final DistributedMember recipient, final EntryEventImpl event,
boolean ifNew, boolean ifOld, Object expectedOldValue, boolean requireOldValue)
throws PrimaryBucketException, ForceReattemptException {
- // boolean forceAck = basicGetWriter() != null
- // || getDistributionAdvisor().adviseNetWrite().size() > 0;
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486463800
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -611,7 +611,8 @@ PartitionedRegionRedundancyTracker getRedundancyTracker() {
return redundancyTracker;
}
- public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucketLockException {
+ public boolean computeWithPrimaryLocked(Object key, Runnable r, boolean tryLock)
Review comment:
You are correct, Darrel, we no longer need it. Jens and I talked about it a bit this. morning and thought it might be beneficial to have the option in the method, but since it's not needed for this PR it makes sense to remove it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5501: GEODE-8339: Redis Rename hangs when servers are killed and revived
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5501:
URL: https://github.com/apache/geode/pull/5501#discussion_r486504827
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -622,7 +623,9 @@ public void computeWithPrimaryLocked(Object key, Runnable r) throws PrimaryBucke
}
try {
- br.doLockForPrimary(false);
+ if (!br.doLockForPrimary(false)) {
Review comment:
just updated it!
----------------------------------------------------------------
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:
users@infra.apache.org