laskoviymishka commented on code in PR #1684:
URL: https://github.com/apache/iceberg-go/pull/1684#discussion_r3739105381
##########
catalog/sql/sql.go:
##########
@@ -455,8 +455,8 @@ func (c *Catalog) CatalogType() catalog.Type {
func (c *Catalog) Close() error {
err := c.reporter.Close()
if c.ownsDB {
- if dbErr := c.db.Close(); dbErr != nil && err == nil {
- err = dbErr
+ if dbErr := c.db.Close(); dbErr != nil {
+ err = errors.Join(err, dbErr)
Review Comment:
This is the right fix for the both-fail case, but it also changes the common
single-failure path: when the reporter closes cleanly and only the DB errors,
`errors.Join(nil, dbErr)` wraps `dbErr` in a `*joinError` rather than returning
it directly like the old code did. `errors.Is`/`As` still traverse fine, but
direct equality and `errors.Unwrap` on the result no longer see `dbErr`.
If we want to keep the old identity for the single-error case:
```go
if dbErr := c.db.Close(); dbErr != nil {
if err != nil {
err = errors.Join(err, dbErr)
} else {
err = dbErr
}
}
```
Not blocking since the test uses `ErrorIs`, but it's a cheap way to avoid
surprising any caller doing value comparisons.
##########
catalog/sql/close_test.go:
##########
@@ -0,0 +1,89 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package sql
+
+import (
+ "context"
+ "database/sql"
+ "database/sql/driver"
+ "errors"
+ "testing"
+
+ "github.com/apache/iceberg-go/metrics"
+ "github.com/stretchr/testify/require"
+ "github.com/uptrace/bun"
+ "github.com/uptrace/bun/dialect/sqlitedialect"
+)
+
+const (
+ closeErrorDriverName = "iceberg_go_sql_close_error"
+ closeErrorReporterName = "iceberg_go_sql_close_reporter"
+)
+
+var closeErrorDBErr error
+
+func init() {
+ sql.Register(closeErrorDriverName, closeErrorDriver{})
+}
+
+type closeErrorDriver struct{}
+
+func (closeErrorDriver) Open(string) (driver.Conn, error) {
+ return closeErrorConn{err: closeErrorDBErr}, nil
+}
+
+type closeErrorConn struct{ err error }
+
+func (c closeErrorConn) Prepare(string) (driver.Stmt, error) { return nil,
driver.ErrSkip }
+func (c closeErrorConn) Close() error { return c.err }
+func (closeErrorConn) Begin() (driver.Tx, error) { return nil,
driver.ErrSkip }
+
+type closeErrorReporter struct{ err error }
+
+func (closeErrorReporter) Report(context.Context, metrics.MetricsReport) {}
+func (r closeErrorReporter) Close() error {
return r.err }
+
+func TestCloseReturnsReporterAndDatabaseErrors(t *testing.T) {
+ dbErr := errors.New("database close")
+ reporterErr := errors.New("reporter close")
+
+ closeErrorDBErr = dbErr
Review Comment:
The reporter side resets itself with `t.Cleanup` (line 70), but
`closeErrorDBErr` never does, so it leaks across runs: under `go test -count=2`
the second run starts with the stale error still set. I'd add `t.Cleanup(func()
{ closeErrorDBErr = nil })` right here to match.
While we're here, `Open` reads this global (line 47) on whatever goroutine
database/sql calls it from, and we write it here with no synchronization, so
`go test -race` on this package would flag it. Threading the error through the
DSN instead of a package global would kill both problems at once, since the
driver would read it per-connection and there'd be nothing shared to reset.
wdyt?
##########
catalog/sql/close_test.go:
##########
@@ -0,0 +1,89 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package sql
+
+import (
+ "context"
+ "database/sql"
+ "database/sql/driver"
+ "errors"
+ "testing"
+
+ "github.com/apache/iceberg-go/metrics"
+ "github.com/stretchr/testify/require"
+ "github.com/uptrace/bun"
+ "github.com/uptrace/bun/dialect/sqlitedialect"
+)
+
+const (
+ closeErrorDriverName = "iceberg_go_sql_close_error"
+ closeErrorReporterName = "iceberg_go_sql_close_reporter"
+)
+
+var closeErrorDBErr error
+
+func init() {
+ sql.Register(closeErrorDriverName, closeErrorDriver{})
+}
+
+type closeErrorDriver struct{}
+
+func (closeErrorDriver) Open(string) (driver.Conn, error) {
+ return closeErrorConn{err: closeErrorDBErr}, nil
+}
+
+type closeErrorConn struct{ err error }
+
+func (c closeErrorConn) Prepare(string) (driver.Stmt, error) { return nil,
driver.ErrSkip }
+func (c closeErrorConn) Close() error { return c.err }
+func (closeErrorConn) Begin() (driver.Tx, error) { return nil,
driver.ErrSkip }
+
+type closeErrorReporter struct{ err error }
+
+func (closeErrorReporter) Report(context.Context, metrics.MetricsReport) {}
+func (r closeErrorReporter) Close() error {
return r.err }
+
+func TestCloseReturnsReporterAndDatabaseErrors(t *testing.T) {
+ dbErr := errors.New("database close")
+ reporterErr := errors.New("reporter close")
+
+ closeErrorDBErr = dbErr
+ metrics.Register(closeErrorReporterName, func(map[string]string)
(metrics.Reporter, error) {
+ return closeErrorReporter{err: reporterErr}, nil
+ })
+ t.Cleanup(func() {
+ metrics.Deregister(closeErrorReporterName)
+ })
+
+ db, err := sql.Open(closeErrorDriverName, "")
+ require.NoError(t, err)
+ require.NoError(t, db.Ping())
Review Comment:
`db.Ping()` is doing more than it looks here: it's the only thing that opens
a connection into the pool, which is what gives `bun.DB.Close()` something to
drain so `closeErrorConn.Close()` actually fires. Worth a one-line comment
saying so, otherwise someone trimming setup could drop it and quietly lose the
db-error-path coverage without any test going red.
--
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]
