READONLY not returning an error in standalone mode broke some test case for Valkey-py.

[PingXie]

          READONLY not returning an error in standalone mode broke some test case for Valkey-py.

Reported by @mkmkme:

Yeah, the test started failing because the test was as simple as

async def test_readonly_invalid_cluster_state(self, r: valkey.Valkey):
    with pytest.raises(exceptions.ValkeyError):
        await r.readonly()

So it assumed readonly command always returns an error

I guess we should have added a check for CLIENT CAPA REDIRECT in the READONLY and READWRITE commands, to keep the old behaviour for old clients.

Originally posted by @zuiderkwast in #325 (comment)

[PingXie]

I think we should back-port this fix too.

[soloestoy]

This is something I used to worry about (not just regarding this feature), when developing new features, we might break existing compatibility, and we cannot unilaterally assess the scope of compatibility.

However, there is another issue: when can we directly introduce breaking changes (bugfix or something)? Continuously providing toggle switches also increases the development burden and the complexity for users during use.

[mkmkme]

Hey!

FWIW it's not like valkey-py API relies on this behaviour or something. It was just an internal test that broke. I have no information regarding if anybody actually relies on this.

[zuiderkwast]

I'm not sure we need to "fix" this. It's not exactly the same situation as for NOSUB, #1228, #1265.

The change to READONLY was that it started to succeed in a scenario where it used to fail before. I'm not sure if this qualifies as a breaking change.

That's what broke the valkey-py test case though, but the test case wasn't testing anything useful. It somehow just wanted coverage for the command.

We did this change in a major version (8.0). If we change it again in 8.0.2, we must really consider it a bug to do that. Actually I vote for not fixing it.

[zuiderkwast]

"should have added a check" when we introduced it not exactly the same as adding it afterwards.

I'm not really against fixing it, but I think we need a better explanation/discussion of why this is a bug or a breaking change before we change it again.

@PingXie @soloestoy What's your analysis?

[PingXie]

Do we know if there are any standalone clients or applications that depend on READONLY failing when the server operates in standalone mode?

I understand it’s hard to get definitive answers here, and we might guess wrong either way. However, if the changes to fix this aren’t too complicated—which seems to be the case—I lean slightly towards making the fix. It feels better to address it now than to worry about it later or document the change, which could open up a can of worms with questions like, “Why?”

However, there is another issue: when can we directly introduce breaking changes (bugfix or something)? Continuously providing toggle switches also increases the development burden and the complexity for users during use.

Yeah I don’t think there’s a one-size-fits-all answer for compatibility decisions. Each case should be evaluated individually IMO. On a high level, I view the right to break compatibility as more of an insurance policy. The more compatibility we can retain (assuming a reasonable amount of effort), the easier it will be for our users to adopt newer versions of Valkey.

[zuiderkwast]

@PingXie Good point, since we're still a new fork and there are other forks and clones too, avoiding incompatibilities should be prioritized.

@soloestoy I think it's very difficult to introduce breaking changes for us, given the current landscape. We can't assume that all Valkey-like databases and managed services will follow our decisions.

[zuiderkwast]

@madolson What do you think, returning an error in a case where we didn't return an error in 8.0, it's a breaking change or a bugfix? It has some similarities to #1369 in that it changes from no error to error.