Skip to content

Commit 4786548

Browse files
authored
Log errors when doing version compatibility check for Cassandra and SQL (temporalio#8100)
## What changed? Changes version compability checks to pass the real logger, so that the error messages would be seeing in the log ## Why? Before, the error was generic and there were no logs indicating the real error (see temporalio#7939): ``` sql schema version compatibility check failed: unable to read DB schema version keyspace/database: temporal error: no usable database connection found ``` With this change, the log would contain the actual error: ``` {"level":"error","ts":"2025-07-25T15:30:49.660-0700","msg":"sql handle: unable to refresh database connection pool","error":"pq: no pg_hba.conf entry for host \"127.0.0.1\", ... ``` The alternative solution would be to return the error, but the design of common/persistence/sql/sqlplugin/db_handle.go suggests we hold the abstraction boundaries, by returning generic errors and logging specific errors. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests
1 parent 21cd558 commit 4786548

File tree

5 files changed

+32
-24
lines changed

5 files changed

+32
-24
lines changed

common/persistence/cassandra/version_checker.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,19 @@ import (
1919
func VerifyCompatibleVersion(
2020
cfg config.Persistence,
2121
r resolver.ServiceResolver,
22+
logger log.Logger,
2223
) error {
23-
24-
if err := checkMainKeyspace(cfg, r); err != nil {
25-
return err
26-
}
27-
return nil
24+
return checkMainKeyspace(cfg, r, logger)
2825
}
2926

3027
func checkMainKeyspace(
3128
cfg config.Persistence,
3229
r resolver.ServiceResolver,
30+
logger log.Logger,
3331
) error {
3432
ds, ok := cfg.DataStores[cfg.DefaultStore]
3533
if ok && ds.Cassandra != nil {
36-
return CheckCompatibleVersion(*ds.Cassandra, r, cassandraschema.Version)
34+
return CheckCompatibleVersion(*ds.Cassandra, r, cassandraschema.Version, logger)
3735
}
3836
return nil
3937
}
@@ -43,13 +41,14 @@ func CheckCompatibleVersion(
4341
cfg config.Cassandra,
4442
r resolver.ServiceResolver,
4543
expectedVersion string,
44+
logger log.Logger,
4645
) error {
4746

4847
session, err := commongocql.NewSession(
4948
func() (*gocql.ClusterConfig, error) {
5049
return commongocql.NewCassandraCluster(cfg, r)
5150
},
52-
log.NewNoopLogger(),
51+
logger,
5352
metrics.NoopMetricsHandler,
5453
)
5554
if err != nil {

common/persistence/sql/version_checker.go

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,38 @@ import (
1313
func VerifyCompatibleVersion(
1414
cfg config.Persistence,
1515
r resolver.ServiceResolver,
16+
logger log.Logger,
1617
) error {
1718

18-
if err := checkMainDatabase(cfg, r); err != nil {
19+
if err := checkMainDatabase(cfg, r, logger); err != nil {
1920
return err
2021
}
2122
if cfg.VisibilityConfigExist() {
22-
return checkVisibilityDatabase(cfg, r)
23+
return checkVisibilityDatabase(cfg, r, logger)
2324
}
2425
return nil
2526
}
2627

2728
func checkMainDatabase(
2829
cfg config.Persistence,
2930
r resolver.ServiceResolver,
31+
logger log.Logger,
3032
) error {
3133
ds, ok := cfg.DataStores[cfg.DefaultStore]
3234
if ok && ds.SQL != nil {
33-
return checkCompatibleVersion(ds.SQL, r, sqlplugin.DbKindMain)
35+
return checkCompatibleVersion(ds.SQL, r, sqlplugin.DbKindMain, logger)
3436
}
3537
return nil
3638
}
3739

3840
func checkVisibilityDatabase(
3941
cfg config.Persistence,
4042
r resolver.ServiceResolver,
43+
logger log.Logger,
4144
) error {
4245
ds, ok := cfg.DataStores[cfg.VisibilityStore]
4346
if ok && ds.SQL != nil {
44-
return checkCompatibleVersion(ds.SQL, r, sqlplugin.DbKindVisibility)
47+
return checkCompatibleVersion(ds.SQL, r, sqlplugin.DbKindVisibility, logger)
4548
}
4649
return nil
4750
}
@@ -50,8 +53,9 @@ func checkCompatibleVersion(
5053
cfg *config.SQL,
5154
r resolver.ServiceResolver,
5255
dbKind sqlplugin.DbKind,
56+
logger log.Logger,
5357
) error {
54-
db, err := NewSQLAdminDB(dbKind, cfg, r, log.NewNoopLogger(), metrics.NoopMetricsHandler)
58+
db, err := NewSQLAdminDB(dbKind, cfg, r, logger, metrics.NoopMetricsHandler)
5559
if err != nil {
5660
return err
5761
}

temporal/fx.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -166,20 +166,20 @@ func ServerOptionsProvider(opts []ServerOption) (serverOptionsProvider, error) {
166166
return serverOptionsProvider{}, err
167167
}
168168

169+
// Logger
170+
logger := so.logger
171+
if logger == nil {
172+
logger = log.NewZapLogger(log.BuildZapLogger(so.config.Log))
173+
}
174+
169175
persistenceConfig := so.config.Persistence
170-
err = verifyPersistenceCompatibleVersion(persistenceConfig, so.persistenceServiceResolver)
176+
err = verifyPersistenceCompatibleVersion(persistenceConfig, so.persistenceServiceResolver, logger)
171177
if err != nil {
172178
return serverOptionsProvider{}, err
173179
}
174180

175181
stopChan := make(chan interface{})
176182

177-
// Logger
178-
logger := so.logger
179-
if logger == nil {
180-
logger = log.NewZapLogger(log.BuildZapLogger(so.config.Log))
181-
}
182-
183183
// ClientFactoryProvider
184184
clientFactoryProvider := so.clientFactoryProvider
185185
if clientFactoryProvider == nil {
@@ -829,13 +829,17 @@ func ServerLifetimeHooks(
829829
lc.Append(fx.StartStopHook(svr.Start, svr.Stop))
830830
}
831831

832-
func verifyPersistenceCompatibleVersion(config config.Persistence, persistenceServiceResolver resolver.ServiceResolver) error {
832+
func verifyPersistenceCompatibleVersion(
833+
cfg config.Persistence,
834+
persistenceServiceResolver resolver.ServiceResolver,
835+
logger log.Logger,
836+
) error {
833837
// cassandra schema version validation
834-
if err := cassandra.VerifyCompatibleVersion(config, persistenceServiceResolver); err != nil {
838+
if err := cassandra.VerifyCompatibleVersion(cfg, persistenceServiceResolver, logger); err != nil {
835839
return fmt.Errorf("cassandra schema version compatibility check failed: %w", err)
836840
}
837841
// sql schema version validation
838-
if err := sql.VerifyCompatibleVersion(config, persistenceServiceResolver); err != nil {
842+
if err := sql.VerifyCompatibleVersion(cfg, persistenceServiceResolver, logger); err != nil {
839843
return fmt.Errorf("sql schema version compatibility check failed: %w", err)
840844
}
841845
return nil

tools/cassandra/version_tests.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (s *VersionTestSuite) TestVerifyCompatibleVersion() {
5252
},
5353
TransactionSizeLimit: dynamicconfig.GetIntPropertyFn(primitives.DefaultTransactionSizeLimit),
5454
}
55-
s.NoError(cassandra.VerifyCompatibleVersion(cfg, resolver.NewNoopResolver()))
55+
s.NoError(cassandra.VerifyCompatibleVersion(cfg, resolver.NewNoopResolver(), log.NewNoopLogger()))
5656
}
5757

5858
func (s *VersionTestSuite) createKeyspace(keyspace string) func() {

tools/sql/clitest/version_tests.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/stretchr/testify/suite"
88
"go.temporal.io/server/common/config"
99
"go.temporal.io/server/common/dynamicconfig"
10+
"go.temporal.io/server/common/log"
1011
persistencetests "go.temporal.io/server/common/persistence/persistence-tests"
1112
persistencesql "go.temporal.io/server/common/persistence/sql"
1213
"go.temporal.io/server/common/primitives"
@@ -106,7 +107,7 @@ func (s *VersionTestSuite) TestVerifyCompatibleVersion() {
106107
},
107108
TransactionSizeLimit: dynamicconfig.GetIntPropertyFn(primitives.DefaultTransactionSizeLimit),
108109
}
109-
s.NoError(persistencesql.VerifyCompatibleVersion(cfg, resolver.NewNoopResolver()))
110+
s.NoError(persistencesql.VerifyCompatibleVersion(cfg, resolver.NewNoopResolver(), log.NewNoopLogger()))
110111
}
111112

112113
func (s *VersionTestSuite) createDatabase(database string) func() {

0 commit comments

Comments
 (0)