-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[ADDED] Delay option to js_cluster_migrate #5903
base: main
Are you sure you want to change the base?
Conversation
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @derekcollison has an opinion on this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
delay
option tojs_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