Reduce catalog contention for RecordMetricsCritical - #4776
Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
🔄 Flaky Test DetectedAnalysis: Three unrelated ClickHouse-cluster e2e subtests each hit the fixed 60s SetupCDCFlowStatusQuery poll cap (UNEXPECTED STATUS TIMEOUT STATUS_SETUP/STATUS_SNAPSHOT) in two of three matrix jobs while the third passed the same suite, matching a known slow-cluster-setup flake the repo already skips a test for — though the PR's catalog-contention changes touch the same status path, so a re-run is warranted to confirm. ✅ Automatically retrying the workflow |
🔄 Flaky Test DetectedAnalysis: Infrastructure flake: the flow-api container failed to start on all 3 attempts because Docker could not bind host port 40003 ("address already in use"), so core services never came up and zero tests ran, while the other two matrix legs on the same commit passed. ✅ Automatically retrying the workflow |
| exceptions.NewCatalogError(fmt.Errorf("unable to establish connection with catalog: %w", err)) | ||
| } | ||
| } | ||
| if pool.Load() == nil { |
There was a problem hiding this comment.
nit: maybe do early return instead of branching two times:
if pool.Load() == nil {
if pool.Load() == nil {
// do smth
}
}
2 participants
When an instance is overloaded with an overly parallel snapshot, RecordMetricsCritical can repeatedly fail to run, not being able to get a catalog connection. This change removes a few silly ways in which catalog was hammered or its client goroutines were delayed.
Fixes:
PEERDB_S3_BYTES_PER_AVRO_FILEandPEERDB_S3_PART_SIZEdynconf, after resolving the above they were the next biggest catalog consumers. We want them to stay hot-configurable but there's no need to reread the values this often. Added a 30 second cache on both, global so workers don't need to re-poll individually. It is making dynconf more messy, but hopefully in a contained way, as the contention problem needs to be solved but redoing dynconf is out of scope right now.Results from running 8x8 snapshots from our mysql perf instance and analyzing 120s traces:
As you can see from the last row, this doesn't fully solve the situation (especially given goroutines can't have priorities). Proper solution is to limit the parallelism allowed/not-discouraged per instance size, but reducing repeat calls doesn't hurt.
Closes DBI-1107