[Scheduler] Generator should respect MaxBufferSize#11072
Conversation
|
|
||
| // A full buffer (MaxBufferSize reached) makes the CHASM generator drop further | ||
| // fires and increment ScheduleInfo.BufferDropped. | ||
| func testBufferOverrunDropsActions(t *testing.T, newContext contextFactory) { |
There was a problem hiding this comment.
Does this repro the issue seen in the wild? I didn't follow what the bug was
There was a problem hiding this comment.
Something I noticed from manual inspection. If someone had a BUFFER_ALL policy, and a short interval (like 1s), and actions that never completed, they could overrun this limit. What would happen in practice (pre-fix) is they'd hit the MS size limit and CHASM would terminate the execution. Backfiller already respects this.
# Conflicts: # chasm/lib/scheduler/generator_tasks_test.go # chasm/lib/scheduler/spec_processor.go
davidporter-id-au
left a comment
davidporter-id-au
left a comment
There was a problem hiding this comment.
lgtm, added a couple of questions but think I eventually answered them myself.
| fmt.Sprintf("buffer overrun, dropped %d actions", result.DroppedCount)) | ||
| logger.Warn("Buffer overrun, dropping actions", | ||
| tag.Int64("dropped-count", result.DroppedCount)) | ||
| if scheduler.Info.BufferDropped == 0 { |
There was a problem hiding this comment.
? should this be greater than zero? I am confused as to what this is.
There was a problem hiding this comment.
oh, I guess this is: emitting metrics in the event that this particular drop occurs, whereas other drops are summed up below, but not subject to this warning.
There was a problem hiding this comment.
It's actually so that we only emit the log warning/event line once/the first time we start overrunning (the metric is always incremented). This is because, if someone has a case where the buffer overruns (e.g., the BUFFER_ALL w/ 1s w/ stuck workflows case I mention), they'll probably keep overrunning until an operator takes some action.
There was a problem hiding this comment.
IIUC, this would only log on the first set of drops. If that's the case why do we want to do that?
There was a problem hiding this comment.
uh, my view must have been stale.
(e.g., the BUFFER_ALL w/ 1s w/ stuck workflows case I mention), they'll probably keep overrunning until an operator takes some action.
So the worry is that it would drown out other event logs? Seems reasonable but could you add a comment?
There was a problem hiding this comment.
So the worry is that it would drown out other event logs?
Yeah. Maybe it's worth it for completeness' sake? I'll keep the guard around the actual logger.Warn call because we don't need that spammed, at least.
There was a problem hiding this comment.
I think it's fine to keep the guard. If I read the log and only saw it once, I'd expect for the event execution to have only dropped this set of actions. Ideally the log would communicate "you'll only see this once per execution". It's also easy enough to look what's producing the log, so a code comment is fine.
| } | ||
|
|
||
| // configWithTweakables returns defaultConfig with its Tweakables mutated by fn. | ||
| func configWithTweakables(fn func(*scheduler.Tweakables)) *scheduler.Config { |
There was a problem hiding this comment.
not a blocking opinion, but any reason to not just set directly?
There was a problem hiding this comment.
not a blocking opinion, but any reason to not just set directly?
Because we'd still need to construct the default config somewhere (for the other tweakable values) and transform it into the per-namespace accessor function. That said, I tidied it up so there's not a separate function for setting the config (since we only need to modify the tweakables).
chaptersix
left a comment
chaptersix
left a comment
There was a problem hiding this comment.
Forgot to submit my previous review. LGTM aside from the conditional check.
3 participants
What changed?
Why?
How did you test it?