@oneiros I think this handles the Node.js / Streaming side of #26571 for redis sentinel mode.

Resolved: @renchap I think the system specs are failing due to something in the redis connection parameters, I'm not entirely what's going wrong here?

Turns out ioredis doesn't support keyPrefix for pub/sub after all. Have opened redis/ioredis#1910 and added code to transparently add and remove the redis keyPrefix for pub/sub commands.

Do we really want to switch to keyPrefix? The fact that it works for some things but not others makes things particularly difficult to follow imho

Having read up on this a bit, I fear that keyPrefix does not work for subscribe etc. because the argument to those functions (conceptually speaking) is not a key but a channel. There might not be a huge difference between the two in practice, but the redis docs at least hint at the fact that those are different concepts. Given that, I am doubtful that redis/ioredis#1910 will be addressed (but maybe a separate channelPrefix might be considered 🤔).

Do we really want to switch to keyPrefix? The fact that it works for some things but not others makes things particularly difficult to follow imho

Having looked at all of the changes in more detail now, I kind of agree with Claire. keyPrefix seems to simplify things, but I can only spot one SET command where it is actually used. And a lot of "noise" is added to simplify a single line.

I wonder if a small utility function (maybe prefixedName(), namespaced() or something similar) might not serve us better. It could be used in place of the redundant seeming template strings and would work the same for the set and the subscribe/unsubscribe operations.

So something like:

function prefixedName(name) {
  return `${redisNamespace}${name}`;
}

// and then later

redisClient.set(prefixedName(`subscribed:${channel}`));

// and

redisSubscribeClient.subscribe(prefixedName(channell));

Do we really want to switch to keyPrefix? The fact that it works for some things but not others makes things particularly difficult to follow imho

@ClearlyClaire I think yes, because it reduces the number of places we manually prefix to just two, making the code more maintainable imo

I knew there was a reason why we didn't just use REDIS_URL exclusively.. there was this issue for the database connection handling about using both DATABASE_URL and DB_PASS, so we'd be introducing variance by not supporting both REDIS_URL and REDIS_PASSWORD simultaneously.

#26295

Have revised the namespacing to not use keyPrefix and instead have two helper methods.

I knew there was a reason why we didn't just use REDIS_URL exclusively.. there was this issue for the database connection handling about using both DATABASE_URL and DB_PASS, so we'd be introducing variance by not supporting both REDIS_URL and REDIS_PASSWORD simultaneously.

#26295

I do not think giving both DATABASE_URL and DB_PASS currently works on the ruby side. And there is no easy way to make it work if I am correct. The separate environment variables are being evaluated in our config/database.yml while DATABASE_URL is an override that rails offers which bypasses whatever is in config/database.yml. So I do not see this changing anytime soon.

Also, I see no need for it. If you want to specify the password separately, simply use the separate environment variables. If you want to give the full configuration in a single variable, use DATABASE_URL. I do not see why we would need to support mixed use cases. It makes the code needlessly complex, makes it harder to explain and makes it easier to introduce subtle errors.

I think the same reasoning applies to redis.

I just approved these changes. I am currently working on adding sentinel support to the ruby side. We should wait with merging this until both PRs are ready and then merge them together.

@oneiros let me know if you need any changes here as you develop out the ruby side of things

The ruby part is ready (well, for review that is) here #31744

I combined both PRs and created a small test setup (3 redis instances, 3 sentinels) locally and everything (inlcuding failover) worked perfectly.