zeroshade commented on code in PR #1430:
URL: https://github.com/apache/iceberg-go/pull/1430#discussion_r3616310460
##########
catalog/sql/sql.go:
##########
@@ -359,6 +363,14 @@ func (c *Catalog) CatalogType() catalog.Type {
return catalog.SQL
}
+// Close releases the catalog's metrics reporter. The SQL catalog is
constructed
+// from a caller-owned database handle, so it does not close that handle; only
+// the reporter is released. Callers holding a [catalog.Catalog] can reach this
+// via a [catalog.Closer] type assertion.
+func (c *Catalog) Close() error { return c.reporter.Close() }
Review Comment:
Non-blocking (fine to fold into the rebase):
This doc says the SQL catalog is "constructed from a caller-owned database
handle" — true for `NewCatalog`, but the `init()` registrar path does its own
`sql.Open(driver, uri)` and hands that DB to `NewCatalog`. For a
registry-loaded catalog (`catalog.Load("sql", …)`) nobody else holds that
handle, so `Close()` — which only releases the reporter — leaks the connection
pool that the new `catalog.Closer` contract implies it cleans up.
Suggestion: track ownership (e.g. an `ownsDB` bool set on the registrar
path) and close the owned `*sql.DB`/bun handle in `Close()`, while keeping the
current non-closing behavior for caller-supplied handles passed to `NewCatalog`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
