1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-06 00:07:44 +01:00
Commit Graph

3 Commits

Author SHA1 Message Date
Nuno Góis
ae375703d2
fix: scheduler job runtime control (#5363)
## PR Description


https://linear.app/unleash/issue/2-1645/address-post-mortem-action-point-all-flags-should-be-runtime

Refactor with the goal of ensuring that flags are runtime controllable,
mostly focused on the current scheduler logic.

This includes the following changes:
 - Moves scheduler into its own "scheduler" feature folder
- Reverts dependency: SchedulerService takes in the MaintenanceService,
not the other way around
- Scheduler now evaluates maintenance mode at runtime instead of relying
only on its mode state (active / paused)
- Favors flag checks to happen inside the scheduled methods, instead of
controlling whether the method is scheduled at all (favor runtime over
startup)
 - Moves "account last seen update" to scheduler
 - Updates tests accordingly
 - Boyscouting

Here's a manual test showing this behavior, where my local instance was
controlled by a remote instance. Whenever I toggle `maintenanceMode`
through a flag remotely, my scheduled functions stop running:


https://github.com/Unleash/unleash/assets/14320932/ae0a7fa9-5165-4c0b-9b0b-53b9fb20de72


Had a look through all of our current flags and it *seems to me* that
they are all used in a runtime controllable way, but would still feel
more comfortable if this was double checked, since it can be complex to
ensure this.

The only exception to this was `migrationLock`, which I believe is OK,
since the migration only happens at the start anyways.

## Discussion / Questions

~~Scheduler `mode` (active / paused) is currently not *really* being
used, along with its respective methods, except in tests. I think this
could be a potential footgun. Should we remove it in favor of only
controlling the scheduler state through maintenance mode?~~ Addressed in
7c52e3f638

~~The config property `disableScheduler` is still a startup
configuration, but perhaps that makes sense to leave as is?~~
[Answered](https://github.com/Unleash/unleash/pull/5363#issuecomment-1819005445)
by @FredrikOseberg, leaving as is.

Are there any other tests we should add?

Is there anything I missed?

Identified some `setInterval` and `setTimeout` that may make sense to
leave as is instead of moving over to the scheduler service:
- ~~`src/lib/metrics` - This is currently considered a `MetricsMonitor`.
Should this be refactored to a service instead and adapt these
setIntervals to use the scheduler instead? Is there anything special
with this we need to take into account? @chriswk @ivarconr~~
[Answered](https://github.com/Unleash/unleash/pull/5363#issuecomment-1820501511)
by @ivarconr, leaving as is.
- ~~`src/lib/proxy/proxy-repository.ts` - This seems to have a complex
and specific logic currently. Perhaps we should leave it alone for now?
@FredrikOseberg~~
[Answered](https://github.com/Unleash/unleash/pull/5363#issuecomment-1819005445)
by @FredrikOseberg, leaving as is.
- `src/lib/services/user-service.ts` - This one also seems to be a bit
more specific, where we generate new timeouts for each receiver id.
Might not belong in the scheduler service. @Tymek
2023-11-21 10:06:38 +00:00
Mateusz Kwasniewski
16190ddca6
feat: disable scheduler for tests (#4496) 2023-08-15 15:58:36 +02:00
Mateusz Kwasniewski
78715e3374
docs: maintanance mode impact (#3858) 2023-05-25 08:28:05 +02:00