update(deps): upgrade gorm to v2 #916
Conversation
|
/cc @breeswish |
Conflicts: pkg/apiserver/slowquery/model.go pkg/apiserver/statement/queries.go
| @@ -141,7 +141,11 @@ func (s *Service) runHandler(c *gin.Context) { | |||
| defer cancel() | |||
|
|
|||
| startTime := time.Now() | |||
| colNames, rows, err := executeStatements(ctx, utils.GetTiDBConnection(c).DB(), req.Statements) | |||
| sqlDB, err := utils.GetTiDBConnection(c).DB() | |||
| if err != nil { | |||
There was a problem hiding this comment.
In what case will this error happen?
There was a problem hiding this comment.
According to https://gorm.io/docs/generic_interface.html. If the underlying database connection is not a *sql.DB, like in a transaction, it will returns error.
pkg/apiserver/user/auth.go
Outdated
| defer db.Close() //nolint:errcheck | ||
| defer func() { | ||
| sqlDB, err := db.DB() | ||
| if err == nil { | ||
| sqlDB.Close() | ||
| } | ||
| }() |
There was a problem hiding this comment.
Why do we need to change to this? (I didn't find this change in gorm v2 release note)
There was a problem hiding this comment.
Cuz there's no Close interface in v2 *gorm.DB. https://pkg.go.dev/gorm.io/gorm#pkg-index
And, is it really necessary to close the connection pool?
go-gorm/gorm#3145
go-gorm/gorm#3834
go-gorm/gorm#3535
go-gorm/gorm#4157
| @@ -42,18 +41,23 @@ func NewDBStore(lc fx.Lifecycle, config *config.Config) (*DB, error) { | |||
|
|
|||
| p := path.Join(config.DataDir, "dashboard.sqlite.db") | |||
| log.Info("Dashboard initializing local storage file", zap.String("path", p)) | |||
| gormDB, err := gorm.Open("sqlite3", p) | |||
| gormDB, err := gorm.Open(sqlite.Open(p), &gorm.Config{ | |||
| Logger: zapgorm2.New(log.L()), | |||
There was a problem hiding this comment.
nice catch! could you test whether it really works? (for example, you can simulate it by producing some error connections). In my current implementation the logger does not really work.
There was a problem hiding this comment.
I think it's works by check cat /tmp/dashboard-data/dashboard.sqlite.db output. And why the logger not work in current implementation?
There was a problem hiding this comment.
You can simulate a database query error, like introducing a syntax error. And then you will find unexpected log output in current implementation.
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
Conflicts: pkg/apiserver/diagnose/diagnose.go
This issue blocks #914-comment