Repository navigation
Conversation
46ca37d to
5ab8eb3
Compare
|
Rebased onto |
The AWS driver opens its own MySQL pools, for the sequencer and for the persistent antispam, and never closes them, so every in-process reopen of a log leaks the pools' goroutines and idle connections. Add an optional DB to Config and to AntispamOpts, mirroring the GCP driver's SpannerClient options. If it is set, Tessera uses it and never closes it, and ignores the pool settings, and Config also ignores its DSN. The antispam checks that the DB is connected to the database its dsn names, since a mismatch would silently break duplicate detection.
5ab8eb3 to
6cfa276
Compare
|
Hi @grempe! IMO the root bug would be better fixed by having Tessera clean up the resources it opened (#1111). The approach in this PR opens up the API considerably, and doesn't resolve the issue for anyone that continues using a DSN. If the issue was resolved with proper termination for any opened resources, would that be sufficient for you? Or is allowing the DB to be injected what you're actually looking for? |
|
Thanks! Yes, proper termination is enough for us. Our log passes only a DSN, and I went with injection only because it mirrored the GCP driver's I've opened #1276, which takes the termination approach for all three drivers: each appender closes what it opened once its context is done and its background jobs have returned, construction that fails closes what it opened, and the antispams get a |
Part of #1251: this adds option 1 there. Pools that Tessera opens itself are still never closed, as on GCP.
The AWS driver could only be given a DSN, and opened its own MySQL pools from it, for the sequencer and for the persistent antispam. It never closed them, so every in-process reopen of a log leaked the pools' goroutines and idle connections.
This change adds an optional
DB *sql.DBtoaws.Configand toAntispamOpts. It mirrorsConfig.SpannerClienton the GCP driver, andAntispamOpts.SpannerClientfrom #1054:DBis set, Tessera uses it, and never closes it. The pool settings (MaxOpenConnsandMaxIdleConns) are ignored, andaws.Configalso ignores itsDSN.SELECT DATABASE(), against the database itsdsnnames, as storage/gcp/antispam: add optional SpannerClient to AntispamOpts #1054 does for its client. A DB connected to another database would otherwise keep a separate, incomplete record of the log's entries, and silently miss duplicates.DBis unset, behaviour is unchanged.Testing
TestAntispamnow runs the full append, follow and duplicate lookup cycle twice, with the antispam's own pool and with a providedDB.TestAntispamSharedDBWrongDatabase: aDBconnected to another, real database is rejected. Without the check,NewAntispamaccepted it.TestStorageUsesProvidedDB: the sequencer usesConfig.DB, not itsDSN, which the test points at an unreachable address. Without the change, the sequencer fails to connect.go test -race ./storage/aws/... -is_mysql_test_optional=falseagainst MySQL 8.4 passes.golangci-lintv2.13.2 reports 0 issues.mainat b2feb46: with caller-owned pools passed in, 5 reopens withMaxIdleConns64 and the antispam left +4 goroutines and +6 MySQL connections, and the connections held at 5–6 from the first round. Without them, it was +36 and +28, growing every round.This touches the same functions as #1244, but the two merge cleanly in either order.
NewAntispamhands a providedDBstraight to a newnewAntispam, asnewMySQLSequencerWithDBdoes for the sequencer, so the pool-settings lines that #1244 changes are left as they are. The new tests sit next to related tests, not at the ends of the files.My first contribution, #1238, adds me to AUTHORS and CONTRIBUTORS under the individual CLA (commit 69ba233), so this PR doesn't repeat that change.
Reviewed and submitted by me, a human. The investigation, the fix and its testing were developed with the assistance of an AI coding agent.