Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change redis.database default to 0? #19

Open
nolta opened this issue Oct 21, 2019 · 3 comments
Open

change redis.database default to 0? #19

nolta opened this issue Oct 21, 2019 · 3 comments

Comments

@nolta
Copy link

nolta commented Oct 21, 2019

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

@jcustenborder
Copy link
Owner

@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?

@Bradleywboggs
Copy link

Bradleywboggs commented Nov 20, 2021

@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
Copy link

Bradleywboggs commented Nov 20, 2021

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)";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants