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 2021/07/09 17:55:43 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
jdeppe-pivotal opened a new pull request #6686:
URL: https://github.com/apache/geode/pull/6686
- Wrap all StripedExecutor.execute calls with
PartitionedRegion.computeWithPrimaryLocked.
- Handle RegionDestroyedException as a result of trying to lock a bucket
while the bucket is moving.
- Improve race condition handling in *ScanExecutor calls
- Rename responds with MOVED when the bucket is moving during the call.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672601801
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -49,6 +49,12 @@ protected void before() {
cache = cacheFactory.create();
server = new GeodeRedisServer("localhost", 0, (InternalCache) cache);
server.setAllowUnsupportedCommands(enableUnsupportedCommands);
+
+ // Ensure that buckets are created up front
+ try {
Review comment:
Regarding the error case where the member info cannot be retrieved - that would result in a 'non-existent' slot (or range of slots) at which point any clients should produce some kind of error if they were trying to write to a missing slot.
Regarding pulling this into `GeodeRedisServer` - are you thinking it should run as part of startup? If so, I'd be wary of that since it would create a bucket imbalance unless we also did a rebalance as part of that lifecycle. As it is, any cluster-aware client will always retrieve the topology as the first thing it does which would implicitly create the buckets. It's still a problem if all members are not up before the very first client starts to connect.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672556499
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -49,6 +49,12 @@ protected void before() {
cache = cacheFactory.create();
server = new GeodeRedisServer("localhost", 0, (InternalCache) cache);
server.setAllowUnsupportedCommands(enableUnsupportedCommands);
+
+ // Ensure that buckets are created up front
+ try {
Review comment:
I think this block should be refactored into a method on GeodeRedisServer.
Also it looks to me like it will be happy even if a bucket can not be created. It looks like we basically need to call SlotAdvisor.getMemberInfo for each bucketId but that method has some retry logic in it and it can fail to get the member info. In that case it logs an error and returns null but this code ends up ignoring those nulls and moving on. I don't know if you want something different here that would fail the startup if one of the memberInfo calls times out.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SScanExecutor.java
##########
@@ -66,12 +67,13 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
RedisKey key = command.getKey();
- if (!getDataRegion(context).containsKey(key)) {
+ RedisData value = context.getRegionProvider().getRedisData(key, null);
Review comment:
this is another place that could be changed to "context.getRegionProvider().getRedisData(key);
if (value.isNull()"
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672556499
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -49,6 +49,12 @@ protected void before() {
cache = cacheFactory.create();
server = new GeodeRedisServer("localhost", 0, (InternalCache) cache);
server.setAllowUnsupportedCommands(enableUnsupportedCommands);
+
+ // Ensure that buckets are created up front
+ try {
Review comment:
I think this block should be refactored into a method on GeodeRedisServer.
Also it looks to me like it will be happy even if a bucket can not be created. It looks like we basically need to call SlotAdvisor.getMemberInfo for each bucketId but that method has some retry logic in it and it can fail to get the member info. In that case it logs an error and returns null but this code ends up ignoring those nulls and moving on. I don't know if you want something different here that would fail the startup if one of the memberInfo calls times out.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SScanExecutor.java
##########
@@ -66,12 +67,13 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
RedisKey key = command.getKey();
- if (!getDataRegion(context).containsKey(key)) {
+ RedisData value = context.getRegionProvider().getRedisData(key, null);
Review comment:
this is another place that could be changed to "context.getRegionProvider().getRedisData(key);
if (value.isNull()"
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r669009122
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/HScanExecutor.java
##########
@@ -67,12 +67,13 @@ public RedisResponse executeCommand(Command command,
}
RedisKey key = command.getKey();
- if (!getDataRegion(context).containsKey(key)) {
+ RedisData value = context.getRegionProvider().getRedisData(key, null);
Review comment:
you could use "getRedisData(key)" to set value and then "value.isNull()" instead of "value == null".
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKeyCommandsFunctionExecutor.java
##########
@@ -136,7 +136,11 @@ public void restore(RedisKey key, long ttl, byte[] data, RestoreOptions options)
@Override
public boolean rename(RedisKey oldKey, RedisKey newKey) {
List<RedisKey> orderedKeys = orderForLocking(oldKey, newKey);
- return stripedExecute(orderedKeys.get(0), () -> rename0(orderedKeys.get(1), oldKey, newKey));
+ return stripedExecute(orderedKeys.get(0), () -> {
+ // Trigger MOVED if necessary
+ getRedisData(newKey);
Review comment:
I'm not sure I understand why this call of getRedisData(newKey) has been added but here are some questions/comments about it. Calling getRedisData(newKey) seems like a race. My understanding is that you are calling it so that it can throw an exception saying the bucket moved. But couldn't move right after you finish calling it and before rename0 is called? And since the get can go to the primary or secondary but the put that rename0 does on newKey goes to the primary isn't possible for that put to fail anyway? Also at this point of time we don't know that we have locked newKey. So if getRedisData(newKey) is needed shouldn't it be done in rename0 after both newKey and oldKey have been locked?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r673538041
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SScanExecutor.java
##########
@@ -66,12 +67,13 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
RedisKey key = command.getKey();
- if (!getDataRegion(context).containsKey(key)) {
+ RedisData value = context.getRegionProvider().getRedisData(key, null);
Review comment:
Fixed
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672576947
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/test/dunit/rules/RedisClusterStartupRule.java
##########
@@ -154,4 +161,47 @@ public void enableDebugLogging(int vmId) {
FastLogger.setDelegating(true);
});
}
+
+ /**
+ * Assuming a redundancy of 1, and at least 3 members, move the given key's primary bucket to a
+ * non-hosting member.
+ */
+ public DistributedMember moveBucketForKey(String key) {
+ return getMember(1).invoke("moveBucketForKey: " + key, () -> {
+ Region<RedisKey, RedisData> r = RedisClusterStartupRule.getCache()
+ .getRegion(RegionProvider.REDIS_DATA_REGION);
+
+ RedisKey redisKey = new RedisKey(key.getBytes());
+ DistributedMember primaryMember = PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ Set<DistributedMember> allHosting = PartitionRegionHelper.getAllMembersForKey(r, redisKey);
+
+ // Returns all members, except the one calling.
+ Set<DistributedMember> allMembers = getCache().getMembers(r);
+ allMembers.add(getCache().getDistributedSystem().getDistributedMember());
+
+ allMembers.removeAll(allHosting);
+ DistributedMember targetMember = allMembers.stream().findFirst().orElseThrow(
+ () -> new IllegalStateException("No non-hosting member found for key: " + key));
+
+ PartitionRegionHelper.moveBucketByKey(r, primaryMember, targetMember, redisKey);
+
+ // Who is the primary now?
+ return PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ });
+ }
+
+ public String getKeyOnServer(String keyPrefix, int vmId) {
Review comment:
Done
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RedisRestoreKeyExistsException.java
##########
@@ -17,7 +17,7 @@
/**
* An exception thrown when the key being restored already exists.
*/
-public class RedisRestoreKeyExistsException extends RuntimeException {
+public class RedisRestoreKeyExistsException extends RedisException {
Review comment:
Done
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -49,6 +49,12 @@ protected void before() {
cache = cacheFactory.create();
server = new GeodeRedisServer("localhost", 0, (InternalCache) cache);
server.setAllowUnsupportedCommands(enableUnsupportedCommands);
+
+ // Ensure that buckets are created up front
+ try {
Review comment:
Regarding the error case where the member info cannot be retrieved - that would result in a 'non-existent' slot (or range of slots) at which point any clients should produce some kind of error if they were trying to write to a missing slot.
Regarding pulling this into `GeodeRedisServer` - are you thinking it should run as part of startup? If so, I'd be wary of that since it would create a bucket imbalance unless we also did a rebalance as part of that lifecycle. As it is, any cluster-aware client will always retrieve the topology as the first thing it does which would implicitly create the buckets. It's still a problem if all members are not up before the very first client starts to connect.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672556499
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -49,6 +49,12 @@ protected void before() {
cache = cacheFactory.create();
server = new GeodeRedisServer("localhost", 0, (InternalCache) cache);
server.setAllowUnsupportedCommands(enableUnsupportedCommands);
+
+ // Ensure that buckets are created up front
+ try {
Review comment:
I think this block should be refactored into a method on GeodeRedisServer.
Also it looks to me like it will be happy even if a bucket can not be created. It looks like we basically need to call SlotAdvisor.getMemberInfo for each bucketId but that method has some retry logic in it and it can fail to get the member info. In that case it logs an error and returns null but this code ends up ignoring those nulls and moving on. I don't know if you want something different here that would fail the startup if one of the memberInfo calls times out.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/set/SScanExecutor.java
##########
@@ -66,12 +67,13 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
RedisKey key = command.getKey();
- if (!getDataRegion(context).containsKey(key)) {
+ RedisData value = context.getRegionProvider().getRedisData(key, null);
Review comment:
this is another place that could be changed to "context.getRegionProvider().getRedisData(key);
if (value.isNull()"
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6686:
URL: https://github.com/apache/geode/pull/6686#issuecomment-877358701
I've tried to break this PR into one commit for core changes and another commit for test-related changes
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6686:
URL: https://github.com/apache/geode/pull/6686#issuecomment-879993993
> Is it possible to make HScanDunitTest and CrashAndNoRepeatDUnitTest a bit more robust? They're failing the stress-new-test tasks and so it's likely they'll become regular flaky failures.
Yes, I'm still working on making them non-flaky
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #6686:
URL: https://github.com/apache/geode/pull/6686
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r669773274
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKeyCommandsFunctionExecutor.java
##########
@@ -136,7 +136,11 @@ public void restore(RedisKey key, long ttl, byte[] data, RestoreOptions options)
@Override
public boolean rename(RedisKey oldKey, RedisKey newKey) {
List<RedisKey> orderedKeys = orderForLocking(oldKey, newKey);
- return stripedExecute(orderedKeys.get(0), () -> rename0(orderedKeys.get(1), oldKey, newKey));
+ return stripedExecute(orderedKeys.get(0), () -> {
+ // Trigger MOVED if necessary
+ getRedisData(newKey);
Review comment:
`stripedExecute` calls `PartitionedRegion.computeWithPrimaryLocked` with the lambda defined here so the bucket shouldn't be able to move as part of this execution sequence.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672576947
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/test/dunit/rules/RedisClusterStartupRule.java
##########
@@ -154,4 +161,47 @@ public void enableDebugLogging(int vmId) {
FastLogger.setDelegating(true);
});
}
+
+ /**
+ * Assuming a redundancy of 1, and at least 3 members, move the given key's primary bucket to a
+ * non-hosting member.
+ */
+ public DistributedMember moveBucketForKey(String key) {
+ return getMember(1).invoke("moveBucketForKey: " + key, () -> {
+ Region<RedisKey, RedisData> r = RedisClusterStartupRule.getCache()
+ .getRegion(RegionProvider.REDIS_DATA_REGION);
+
+ RedisKey redisKey = new RedisKey(key.getBytes());
+ DistributedMember primaryMember = PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ Set<DistributedMember> allHosting = PartitionRegionHelper.getAllMembersForKey(r, redisKey);
+
+ // Returns all members, except the one calling.
+ Set<DistributedMember> allMembers = getCache().getMembers(r);
+ allMembers.add(getCache().getDistributedSystem().getDistributedMember());
+
+ allMembers.removeAll(allHosting);
+ DistributedMember targetMember = allMembers.stream().findFirst().orElseThrow(
+ () -> new IllegalStateException("No non-hosting member found for key: " + key));
+
+ PartitionRegionHelper.moveBucketByKey(r, primaryMember, targetMember, redisKey);
+
+ // Who is the primary now?
+ return PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ });
+ }
+
+ public String getKeyOnServer(String keyPrefix, int vmId) {
Review comment:
Done
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RedisRestoreKeyExistsException.java
##########
@@ -17,7 +17,7 @@
/**
* An exception thrown when the key being restored already exists.
*/
-public class RedisRestoreKeyExistsException extends RuntimeException {
+public class RedisRestoreKeyExistsException extends RedisException {
Review comment:
Done
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r670664004
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKeyCommandsFunctionExecutor.java
##########
@@ -136,7 +136,11 @@ public void restore(RedisKey key, long ttl, byte[] data, RestoreOptions options)
@Override
public boolean rename(RedisKey oldKey, RedisKey newKey) {
List<RedisKey> orderedKeys = orderForLocking(oldKey, newKey);
- return stripedExecute(orderedKeys.get(0), () -> rename0(orderedKeys.get(1), oldKey, newKey));
+ return stripedExecute(orderedKeys.get(0), () -> {
+ // Trigger MOVED if necessary
+ getRedisData(newKey);
Review comment:
Jens and I talked about this and it turns out we now require that the two keys passed to rename are on the same primary bucket. That is why having done the first stripedExecute will have locked the primary even though the first stripedExecute may have been done on oldKey not newKey. We agreed that it would be better to do a check to ensure that both keys are on the same primary and then this extra getRedisData(newKey) call is not needed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r669009122
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/hash/HScanExecutor.java
##########
@@ -67,12 +67,13 @@ public RedisResponse executeCommand(Command command,
}
RedisKey key = command.getKey();
- if (!getDataRegion(context).containsKey(key)) {
+ RedisData value = context.getRegionProvider().getRedisData(key, null);
Review comment:
you could use "getRedisData(key)" instead of "getRedisData(key, null)" and then "value.isNull()" instead of "value == null".
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r672576947
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/test/dunit/rules/RedisClusterStartupRule.java
##########
@@ -154,4 +161,47 @@ public void enableDebugLogging(int vmId) {
FastLogger.setDelegating(true);
});
}
+
+ /**
+ * Assuming a redundancy of 1, and at least 3 members, move the given key's primary bucket to a
+ * non-hosting member.
+ */
+ public DistributedMember moveBucketForKey(String key) {
+ return getMember(1).invoke("moveBucketForKey: " + key, () -> {
+ Region<RedisKey, RedisData> r = RedisClusterStartupRule.getCache()
+ .getRegion(RegionProvider.REDIS_DATA_REGION);
+
+ RedisKey redisKey = new RedisKey(key.getBytes());
+ DistributedMember primaryMember = PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ Set<DistributedMember> allHosting = PartitionRegionHelper.getAllMembersForKey(r, redisKey);
+
+ // Returns all members, except the one calling.
+ Set<DistributedMember> allMembers = getCache().getMembers(r);
+ allMembers.add(getCache().getDistributedSystem().getDistributedMember());
+
+ allMembers.removeAll(allHosting);
+ DistributedMember targetMember = allMembers.stream().findFirst().orElseThrow(
+ () -> new IllegalStateException("No non-hosting member found for key: " + key));
+
+ PartitionRegionHelper.moveBucketByKey(r, primaryMember, targetMember, redisKey);
+
+ // Who is the primary now?
+ return PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ });
+ }
+
+ public String getKeyOnServer(String keyPrefix, int vmId) {
Review comment:
Done
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RedisRestoreKeyExistsException.java
##########
@@ -17,7 +17,7 @@
/**
* An exception thrown when the key being restored already exists.
*/
-public class RedisRestoreKeyExistsException extends RuntimeException {
+public class RedisRestoreKeyExistsException extends RedisException {
Review comment:
Done
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -49,6 +49,12 @@ protected void before() {
cache = cacheFactory.create();
server = new GeodeRedisServer("localhost", 0, (InternalCache) cache);
server.setAllowUnsupportedCommands(enableUnsupportedCommands);
+
+ // Ensure that buckets are created up front
+ try {
Review comment:
Regarding the error case where the member info cannot be retrieved - that would result in a 'non-existent' slot (or range of slots) at which point any clients should produce some kind of error if they were trying to write to a missing slot.
Regarding pulling this into `GeodeRedisServer` - are you thinking it should run as part of startup? If so, I'd be wary of that since it would create a bucket imbalance unless we also did a rebalance as part of that lifecycle. As it is, any cluster-aware client will always retrieve the topology as the first thing it does which would implicitly create the buckets. It's still a problem if all members are not up before the very first client starts to connect.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6686: GEODE-9368: Changes to support better resiliency for HA scenarios
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6686:
URL: https://github.com/apache/geode/pull/6686#discussion_r671517604
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/RedisRestoreKeyExistsException.java
##########
@@ -17,7 +17,7 @@
/**
* An exception thrown when the key being restored already exists.
*/
-public class RedisRestoreKeyExistsException extends RuntimeException {
+public class RedisRestoreKeyExistsException extends RedisException {
Review comment:
Very minor point, but might this class be better located in the `org.apache.geode.redis.internal.data` package?
##########
File path: geode-apis-compatible-with-redis/src/commonTest/java/org/apache/geode/test/dunit/rules/RedisClusterStartupRule.java
##########
@@ -154,4 +161,47 @@ public void enableDebugLogging(int vmId) {
FastLogger.setDelegating(true);
});
}
+
+ /**
+ * Assuming a redundancy of 1, and at least 3 members, move the given key's primary bucket to a
+ * non-hosting member.
+ */
+ public DistributedMember moveBucketForKey(String key) {
+ return getMember(1).invoke("moveBucketForKey: " + key, () -> {
+ Region<RedisKey, RedisData> r = RedisClusterStartupRule.getCache()
+ .getRegion(RegionProvider.REDIS_DATA_REGION);
+
+ RedisKey redisKey = new RedisKey(key.getBytes());
+ DistributedMember primaryMember = PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ Set<DistributedMember> allHosting = PartitionRegionHelper.getAllMembersForKey(r, redisKey);
+
+ // Returns all members, except the one calling.
+ Set<DistributedMember> allMembers = getCache().getMembers(r);
+ allMembers.add(getCache().getDistributedSystem().getDistributedMember());
+
+ allMembers.removeAll(allHosting);
+ DistributedMember targetMember = allMembers.stream().findFirst().orElseThrow(
+ () -> new IllegalStateException("No non-hosting member found for key: " + key));
+
+ PartitionRegionHelper.moveBucketByKey(r, primaryMember, targetMember, redisKey);
+
+ // Who is the primary now?
+ return PartitionRegionHelper.getPrimaryMemberForKey(r, redisKey);
+ });
+ }
+
+ public String getKeyOnServer(String keyPrefix, int vmId) {
Review comment:
This method seems like it could use some documentation around its intended use, since it's not immediately clear how it's supposed to work.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org