jdeppe-pivotal commented on a change in pull request #6960:
URL: https://github.com/apache/geode/pull/6960#discussion_r727277195
##########
File path:
geode-for-redis/src/acceptanceTest/java/org/apache/geode/redis/GeodeRedisServerStartupDUnitTest.java
##########
@@ -143,4 +147,84 @@ public void startupWorksGivenNoBindAddress() {
assertThat(cluster.getRedisPort(server))
.isNotEqualTo(GeodeRedisServer.DEFAULT_REDIS_SERVER_PORT);
}
+
+ @Test
+ public void startupUsesDefaultRegionName_whenSystemPropertyNotSet() {
+ MemberVM server = cluster.startServerVM(0, s -> s
+ .withProperty(REDIS_ENABLED, "true"));
+
+ assertThat(cluster.getRedisRegionName(server))
+ .isEqualTo(GeodeRedisServer.DEFAULT_REDIS_REGION_NAME);
+ }
+
+ @Test
+ public void startupUsesSpecifiedRegionName_whenSystemPropertyIsSet() {
+ MemberVM server = cluster.startServerVM(0, s -> s
+ .withProperty(REDIS_ENABLED, "true")
+ .withSystemProperty(REDIS_REGION_NAME, "regionName"));
+
+ assertThat(cluster.getRedisRegionName(server)).isEqualTo("regionName");
+ }
+
+ @Test
+ public void
startupUsesDefaultRegionName_whenSystemPropertySetToEmptyString() {
+ MemberVM server = cluster.startServerVM(0, s -> s
+ .withProperty(REDIS_ENABLED, "true")
+ .withSystemProperty(REDIS_REGION_NAME, ""));
+
+ assertThat(cluster.getRedisRegionName(server))
+ .isEqualTo(GeodeRedisServer.DEFAULT_REDIS_REGION_NAME);
+ }
+
+ @Test
+ public void
startupUsesSpecifiedRegionName_whenSystemPropertySetOnMultipleServers() {
+ MemberVM server1 = cluster.startServerVM(0, s -> s
+ .withProperty(REDIS_ENABLED, "true")
+ .withProperty(REDIS_PORT, "0")
+ .withSystemProperty(REDIS_REGION_NAME, "region"));
+
+ MemberVM server2 = cluster.startServerVM(1, s -> s
+ .withProperty(REDIS_ENABLED, "true")
+ .withProperty(REDIS_PORT, "0")
+ .withSystemProperty(REDIS_REGION_NAME, "region"));
+
+ assertThat(cluster.getRedisRegionName(server1))
+ .isEqualTo(cluster.getRedisRegionName(server2))
+ .isEqualTo("region");
+ }
+
+ @Test
+ public void
startupUsesSpecifiedRegionName_whenSystemPropertySetToDifferentValuesOnMultipleServers()
{
Review comment:
I'm not sure that this test is really relevant. It's testing for
something that is possible but not supported/allowed. However, in such a
scenario we don't produce any errors or alert the user which is something a
test might assert on. Since we don't do that, a failure of this test could only
point to a test issue and never point at a product issue.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -156,6 +167,10 @@ public int getPort() {
return serverPort;
}
+ public String getRegionName() {
Review comment:
The Netty server should not have any knowledge of the region - since the
notion of a region has no relevance to the netty server we shouldn't couple it
like this.
##########
File path:
geode-for-redis/src/commonTest/java/org/apache/geode/test/dunit/rules/RedisClusterStartupRule.java
##########
@@ -167,17 +179,17 @@ public void enableDebugLogging(int vmId) {
* non-hosting member.
*/
public DistributedMember moveBucketForKey(String key) {
- return moveBucketForKey(key, null);
+ return moveBucketForKey(key, null, DEFAULT_REDIS_REGION_NAME);
Review comment:
Is there a particular reason to do this? - since the region parameter is
just a static here it can just be left as it was.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -51,6 +51,7 @@
* The default Redis port as specified by their protocol, {@code
DEFAULT_REDIS_SERVER_PORT}
*/
public static final int DEFAULT_REDIS_SERVER_PORT = 6379;
+ public static final String DEFAULT_REDIS_REGION_NAME = "REDIS_DATA";
Review comment:
Since we already have a definition of the region name in
`RegionProvider` I don't think we should add anything here. That also
introduces 2 sources of 'truth' for how to reference the region name.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
