[ADDED] Delay option to js_cluster_migrate #5903
Conversation
piotrpio
force-pushed
the
jetstream-migrate-delay
branch
from
914dfe1
to
2adbc4a
Compare
This adds a `delay` option to `js_cluster_migrate` as an option in nested object. The assets assets will only be migrated after the specified delay (rather than immediately, which is the default). Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio
force-pushed
the
jetstream-migrate-delay
branch
from
2adbc4a
to
8ac0983
Compare
|
This ready for review @piotrpio ? |
| @@ -2746,6 +2750,16 @@ func parseRemoteLeafNodes(v any, errors *[]error, warnings *[]error) ([]*RemoteL | |||
| remote.Websocket.NoMasking = v.(bool) | |||
| case "jetstream_cluster_migrate", "js_cluster_migrate": | |||
| remote.JetStreamClusterMigrate = true | |||
| migrateConfig, ok := v.(map[string]any) | |||
There was a problem hiding this comment.
Although it was this way before, currently setting jetstream_cluster_migrate: false in the config looks like it will do the wrong thing and set to true anyway. Do we want to fix this?
Might also be better just to check for bool vs string, parse the string as duration, rather than having to open up a new map. That way you could do any of:
jetstream_cluster_migrate: true
jetstream_cluster_migrate: 30s
jetstream_cluster_migrate: false
WDYT?
There was a problem hiding this comment.
Maybe @derekcollison has an opinion on this too.
There was a problem hiding this comment.
I would prefer leader_migrate_delay, although that is pretty long winded for me but is more descriptive IMO. It is a value that defaults to 0, meaning instant, and you can set to a value that means after Nsecs of being disconnected from leafnode we will migrate leaders.
There was a problem hiding this comment.
@neilalexander I agree but I wonder how much of a breaking change that would be js_cluster_enabled: false seems like a bug which can be fixed, but right now e.g. js_cluster_migrate: enabled also works (or any other value really).
@derekcollison with leader_migrate_delay you would see this as a separate option in remote or an option in object on js_cluster_migrate (as I did it here with delay)?
|
@derekcollison Yes, it's ready for review, I was waiting for the tests to pass and did not change the state. |
This adds a
delayoption tojs_cluster_migrateas an option in nested object. The assets assets will only be migrated after the specified delay (rather than immediately, which is the default).Signed-off-by: Piotr Piotrowski piotr@synadia.com