Add Redis DSL connector - #807
Conversation
johntomcat7408-cmyk
left a comment
johntomcat7408-cmyk
left a comment
There was a problem hiding this comment.
I checked this against #799 and left a few inline comments on authentication, partition handling, and config logging.
The issue also asks for a Testcontainers test. The current jedis-mock tests do not cover real Redis authentication or the source partition check. The branch currently conflicts with master and needs a rebase.
Is the column-mapped hash mode intended to be sink-only? If so, the source should reject that configuration and the docs should state the limitation.
| String password = conf.getString(RedisConfigKeys.GEAFLOW_DSL_REDIS_PASSWORD); | ||
| int timeout = conf.getInteger(RedisConfigKeys.GEAFLOW_DSL_REDIS_CONNECTION_TIMEOUT); | ||
| GenericObjectPoolConfig poolConfig = new GenericObjectPoolConfig(); | ||
| return new JedisPool(poolConfig, host, port, timeout, user, password); |
There was a problem hiding this comment.
Both options default to empty strings. With Jedis 3.3, this overload still attempts authentication because user is non-null, so a Redis instance without authentication rejects the connection. Please select the no-auth, password-only, or ACL constructor according to the configured values.
| } | ||
|
|
||
| @Override | ||
| public void setIndex(int index, int parallel) { |
There was a problem hiding this comment.
GeaFlowTableSourceFunction calls listPartitions(parallelism) repeatedly and compares the returned lists. RedisPartition currently uses identity equality, so an unchanged partition list is reported as modified. setIndex also replaces the bucket used by accept(), which can lead to missed or duplicate keys. Please keep the logical bucket unchanged and implement equals/hashCode from it.
|
|
||
| @Override | ||
| public void init(Configuration tableConf, StructType schema) { | ||
| LOGGER.info("Prepare redis sink with config: {}, schema: {}", tableConf, schema); |
There was a problem hiding this comment.
Configuration.toString() includes the raw values. This log will expose geaflow.dsl.redis.password when it is configured. Please remove the full configuration from this message or mask the password.
2 participants
What changes were proposed in this pull request?
This PR adds a new Redis DSL connector module,
geaflow-dsl-connector-redis. For issue #799:The Redis connector supports:
RedisTableSink.RedisTableSource.SCANcommand.REDIS.This PR also adds Redis connector configuration keys, CN/EN documentation with SQL examples, and unit tests based on
jedis-mock. The connector reuses the existing Jedis dependency already managed by the project.How was this PR tested?
Verified with:
mvn -pl geaflow-dsl/geaflow-dsl-connector/geaflow-dsl-connector-redis -am test -Dtest=RedisTableConnectorTest -Dsurefire.failIfNoSpecifiedTests=false -DskipITs Fixes https://github.com/apache/geaflow/issues/799