kafka-connect-redis icon indicating copy to clipboard operation
kafka-connect-redis copied to clipboard

change redis.database default to 0?

Open nolta opened this issue 5 years ago • 3 comments

I'm wondering why redis.database defaults to 1 instead of 0. When you start redis-cli without options, the default database is 0.

nolta avatar Oct 21 '19 18:10 nolta

@nolta Interesting. The docker container seems to be fine with this. During unit tests I actually build a docker container and interact with it. This is the docker-compose.yml it is using for testing. Is this still an issue?

jcustenborder avatar Jan 22 '20 20:01 jcustenborder

@jcustenborder I can confirm that this is still an issue. When my team first tried the connector out, it took a bit to realize that there was a mismatch between the default database setting for the connector (1) and the redis-cli (0).
Here's some code and a couple screenshots to illustrate: I ran this test against my local redis with no modification to the defaults.

    @Test
    public void testRedisSessionMSET() throws InterruptedException {
        RedisSinkConnectorConfig config = new RedisSinkConnectorConfig(props);
        RedisSession sesh = new RedisSessionFactoryImpl().create(config);
        Map<byte[],  byte[]> redisVals = new HashMap<>();
        redisVals.put("key".getBytes(StandardCharsets.UTF_8), "value".getBytes(StandardCharsets.UTF_8));
        RedisFuture<String> res = sesh.asyncCommands().mset(redisVals);
        res.wait();
    }

Here's the output of redis cli: note that initially when GET is is run, I get nil and then when I swapDb then I get the expected result:

image

It's not a huge deal, and those with more redis experience would probably be able to spot this mismatch quickly, but changing the default to 0 could make the initial "trying-it-out" more straightforward and successful out of the box for people shopping for solutions.

I'll test locally with the repo and see if tests fail as a result of a change to the default.

Bradleywboggs avatar Nov 20 '21 11:11 Bradleywboggs

I confirmed that changing the default value for redis.database doesn't affect whether tests pass or not; however, on reflection, it's definitely possible that changing the default would break existing code relying on the connector. So, I think probably the change that should happen is one of documentation

Something like:

  static final String DATABASE_DOC = "Redis database to connect to. Defaults to 1 (Note that the redis-cli defaults to 0)";

Bradleywboggs avatar Nov 20 '21 12:11 Bradleywboggs