spring-projects / spring-data-geode

Spring Data support for Apache Geode
Apache License 2.0
51 stars 39 forks source link

main-next is failing to compile due to missing Redis related Apache Geode properties #592

Closed upthewaterspout closed 2 years ago

upthewaterspout commented 2 years ago

The main-next branch is failing to compile with the below error. This is likely due to changes made to geode in GEODE-10126, where these Geode properties were removed. In Geode these properties were marked experimental in Geode 1.14, so removing them was not considered a breaking change. Spring Data Geode should remove references to these properties for the next release that is intended to work with Geode 1.15.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (java-compile) on project spring-data-geode: Compilation failure: Compilation failure: 
[ERROR] /Users/upthewaterspout/Documents/Code/spring-data-geode/spring-data-geode/src/main/java/org/springframework/data/gemfire/GemFireProperties.java:[103,43] error: cannot find symbol
[ERROR]   symbol:   variable GEODE_FOR_REDIS_BIND_ADDRESS
[ERROR]   location: interface ConfigurationProperties
[ERROR] /Users/upthewaterspout/Documents/Code/spring-data-geode/spring-data-geode/src/main/java/org/springframework/data/gemfire/GemFireProperties.java:[104,38] error: cannot find symbol
[ERROR]   symbol:   variable GEODE_FOR_REDIS_ENABLED
[ERROR]   location: interface ConfigurationProperties
[ERROR] /Users/upthewaterspout/Documents/Code/spring-data-geode/spring-data-geode/src/main/java/org/springframework/data/gemfire/GemFireProperties.java:[105,35] error: cannot find symbol
[ERROR]   symbol:   variable GEODE_FOR_REDIS_PORT
[ERROR]   location: interface ConfigurationProperties
[ERROR] /Users/upthewaterspout/Documents/Code/spring-data-geode/spring-data-geode/src/main/java/org/springframework/data/gemfire/GemFireProperties.java:[106,47] error: cannot find symbol
[ERROR]   symbol:   variable GEODE_FOR_REDIS_REDUNDANT_COPIES
[ERROR]   location: interface ConfigurationProperties
[ERROR] /Users/upthewaterspout/Documents/Code/spring-data-geode/spring-data-geode/src/main/java/org/springframework/data/gemfire/GemFireProperties.java:[107,39] error: cannot find symbol
[ERROR]   symbol:   variable GEODE_FOR_REDIS_USERNAME
[ERROR]   location: interface ConfigurationProperties
[ERROR] -> [Help 1]
jxblum commented 2 years ago

I am curious why the Redis port property was removed?

Configuring the embedded Redis service port for which Apache Geode will listen for Redis client connections seems like a sensible property to me, particularly given systems may share the same port number, even if unlikely.

More likely is the fact that users and customers alike will be running Apache Geode (and by extension, VMware GemFire) behind a corporate firewall, and will need to configure the ports of some of GemFire/Geode's embedded services, like Redis, or the HTTP service, and so on.

While Redis's default port (6379) might work in the majority of the cases, it won't work in all cases.

Thoughts?

jxblum commented 2 years ago

Additionally, if I remember correctly, the value of the Redis port property was even used to enable or disable the embedded Redis (server) service, such as whether the Redis port was 0 or not.

It is the new thinking that the embedded Redis (server) service will be enabled by default if the new Redis module is included on the classpath of the Apache Geode server(s) in the cluster?

I still might advise that even if the Redis module is on the classpath of the Geode servers in the cluster, that users and (GemFire) customers alike might still want a way to (conditionally) disable the embedded Redis service in certain uses case while leaving the configuration of their Geode servers (e.g. classpath) unchanged.

This is particularly important managed environments, like PCF or K8S, where the configuration of GemFire/Geode is not as readily or as easily modifiable.

It certainly should be possible to start and stop the embedded Redis service without necessarily starting and stopping the GemFire/Geode node (server) in which the embedded Redis service is running. How does a user/customer do this?

upthewaterspout commented 2 years ago

The main reason to remove the geode properties is that we moved the configuration for the redis server into the redis module itself. This further decouples the geode-for-redis module from geode-core.

The same properties are still available as system properties in the geode-for-redis module, so users can still turn geode-for-redis off if they want to, even if it is on the classpath. See this PR for the new property names.

upthewaterspout commented 2 years ago

Also, there is currently a discussion about removing geode-for-redis completely from Apache Geode - see this thread - https://lists.apache.org/thread/7m23h9r0tf536g414bwjsplqh1qv2ct0 .

jxblum commented 2 years ago

SDG supports an Apache Geode server in the capacity as a Redis server using (enabling) the embedded Redis service, so these properties (and the corresponding support in SDG using the @EnableRedisServer annotation; see docs), cannot simply be removed.

Rather, SDG must now optionally include the new Redis module dependency, which is?

Would this be org.apache.geode:geode-for-redis, and matching the version of Geode (perhaps, 1.15.0)?

Is there a GitHub repo or README that talks about how to use the new Redis module?

Sorry, I am currently sifting through all of this now.

jxblum commented 2 years ago

So, it seemed as though this worked for the time being:

<dependency>
    <groupId>org.apache.geode</groupId>
    <artifactId>geode-for-redis</artifactId>
    <version>${geode.version}</version>
    <optional>true</optional>
</dependency>
jxblum commented 2 years ago

Why is the org.apache.geode.redis.internal.RedisConfiguration class and the org.apache.geode.redis.internal.SystemPropertyBasedRedisConfiguration classes "internal"?

Additionally, the o.a.g.redis.internal.SystemPropertyBasedRedisConfiguration class's declaration of the Apache Geode properties used to configure the embedded Redis service is inconsistent with Apache Geode's (standard) ConfigurationProperties class where the gemfire. prefix is not used in the property names.

It would be better to simply have a "public" RedisConfiguration class declaring the property names with, perhaps, a utility method to get the property as a System property as needed.

For example:

package org.apache.geode.redis.config;

enum RedisConfiguration {

  // ...
  ENABLED("geode-for-redis-enabled"),
  PORT("geode-for-redis-port");

  private final String propertyName;

  RedisConfiguration(String propertyName) {
    this.propertyName = propertyName;
  }

  public String getSystemProperty() {
    return this.propertyName;
  }

  public String asSystemProperty() {
    return "gemfire.".concat(getPropertyName());
  }

  // ...
}