add noise to cron scheduling #20665
add noise to cron scheduling #20665
Conversation
octavia-squidington-iv
added
area/platform
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
| return new ScheduleRetrieverOutput(timeToWait); | ||
| } catch (final ParseException e) { | ||
| throw (DateTimeException) new DateTimeException(e.getMessage()).initCause(e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void addSchedulingNoiseForAllowListedWorkspace(Duration timeToWait, StandardSync standardSync) { |
There was a problem hiding this comment.
is this activity used in AWS data plane? Seems it depends on ConfigRepository but data plane can't talk to db directly
There was a problem hiding this comment.
I believe that this activity runs in the control plane.
There was a problem hiding this comment.
(In any case the dependency already exists, so it's a bit unrelated to this PR.)
|
this is a truly baller idea @mfsiega-airbyte nice find I still think there is a more complete "correct" answer, but this is some pretty solid duct tape |
|
@supertopher I can claim 0% of the idea, that was all @malikdiarra! |
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
| @@ -323,6 +325,36 @@ void testCronScheduleMinimumInterval() throws IOException, JsonValidationExcepti | |||
| .hasHours(24); | |||
| } | |||
|
|
|||
| @Test | |||
| @DisplayName("Test that for specific workspace ids, we add some noise in the cron scheduling") | |||
| private final static long MS_PER_SECOND = 1000L; | ||
| private final static long MIN_CRON_INTERVAL_SECONDS = 60; | ||
| private static final Set<UUID> SCHEDULING_NOISE_WORKSPACE_IDS = Set.of( | ||
| // Testing |
There was a problem hiding this comment.
Add a TODO to move this to env vars?
There was a problem hiding this comment.
Hoping we'll get rid of it altogether :)
| // We really do want to add some scheduling noise for this connection. | ||
| final long minutesToWait = (long) (Math.random() * SCHEDULING_NOISE_CONSTANT); | ||
| LOGGER.debug("Adding {} minutes noise to wait", minutesToWait); | ||
| // Note: we add an extra second to make the unit tests pass in case `minutesToWait` was 0. |
There was a problem hiding this comment.
nitpick: is this necessary?
There was a problem hiding this comment.
Without it the test fails 1/15 times (because Math.random() * SCHEDULING_NOISE_CONSTANT is <1, so it rounds to zero) which is pretty flaky for a unit test. It's a bit of a hack (hence the comment) - a better thing would be to inject a randomness provider or do some seeding of Math.random but this was a lot simpler.
|
/approve-and-merge reason="mitigates a paging oncall issue" |
|
This code is at peace with itself |
What
Adds 0-15 minutes of random noise to cron scheduling.
The time is always added, never subtracted. It's restricted to an allow-listed set of workspaces (empty in this PR).