Toy server: support maintenance-notifications - #3191
Conversation
|
@philon-msft my plan is to turn on SCH for AMR (in the options-provider) pre-emptively - i.e. ahead of AMR actually supporting it; this would be in "auto" mode, which means "only try in RESP3 mode; if it gets rejected: meh, no problem" - however, before I do this, I'd want to validate against a real AMR endpoint that this doesn't break anything. Would you be able to lend me something to test this against, or otherwise help me validate this? If we need to do "turn it on in AMR if the server reports version > XXX, otherwise don't because it breaks the proxy/whatever" then that's totally fine too: we'll make it work - I just want to not break your users. Specifically, this would issue an additional command:
|
…events Deliberately on RedisServer rather than a bespoke test subclass. The opt-in is an ordinary command, so any test should be able to use it, and a server that never sends a notification is the normal case - every OSS, Valkey and Garnet build behaves that way, and so will our own docker topology. The interesting behaviour is all client-side, so the fake should not be a special place. CLIENT MAINT_NOTIFICATIONS <ON|OFF> [parameter value ...] per the contract: a bare ON is valid and means "server defaults", moving-endpoint-type is the only parameter defined so far, and its five values are validated. Unknown parameters are refused rather than ignored - the client is asking the server to do something specific, and silently not doing it is worse than saying no. State is per connection, including a count, since re-arming after a reconnect is a requirement and a count is what distinguishes that from having opted in once. MaintenanceNotifications selects how the server answers: accept, reject as an unknown subcommand (what a server that never heard of it does), or reject as disabled (what one with the feature flag off does). A client has to survive all three, so a test has to be able to ask for all three. Sending covers MOVING with or without an address, the shard-scoped and slot-scoped families, explicit or generated sequence ids - the contract never defines those, so repeating one deliberately is part of what the fake owes us - and a raw-push hook for malformed frames. Notifications go only to connections that opted in; a raw push is not gated, which is the contrast the tests assert. Two bugs the tests caught: the count returned was clients *visited* rather than sent to, because ForAllClients' Action overload returns one per client regardless; and an assertion comparing against ClientCount was racy, since under RESP2 the subscription connection can register between the send and the read.
88b79ce to
0f3ae13
Compare
support the maintenance-notification opt-in, and sending events
Deliberately on RedisServer rather than a bespoke test subclass. The opt-in is an ordinary command, so any test should be able to use it, and a server that never sends a notification is the normal case - every OSS, Valkey and Garnet build behaves that way, and so will our own docker topology. The interesting behaviour is all client-side, so the fake should not be a special place.
CLIENT MAINT_NOTIFICATIONS <ON|OFF> [parameter value ...] per the contract: a bare ON is valid and means "server defaults", moving-endpoint-type is the only parameter defined so far, and its five values are validated. Unknown parameters are refused rather than ignored - the client is asking the server to do something specific, and silently not doing it is worse than saying no. State is per connection, including a count, since re-arming after a reconnect is a requirement and a count is what distinguishes that from having opted in once.
MaintenanceNotifications selects how the server answers: accept, reject as an unknown subcommand (what a server that never heard of it does), or reject as disabled (what one with the feature flag off does). A client has to survive all three, so a test has to be able to ask for all three.
Sending covers MOVING with or without an address, the shard-scoped and slot-scoped families, explicit or generated sequence ids - the contract never defines those, so repeating one deliberately is part of what the fake owes us - and a raw-push hook for malformed frames. Notifications go only to connections that opted in; a raw push is not gated, which is the contrast the tests assert.
Two bugs the tests caught: the count returned was clients visited rather than sent to, because ForAllClients' Action overload returns one per client regardless; and an assertion comparing against ClientCount was racy, since under RESP2 the subscription connection can register between the send and the read.
Checklist