New functions to close a mysql connection. #10355
Conversation
This is a possible fix for issue #9415. Added a function in the mysql file and a method for the metricset to close the db too.
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
metricbeat/module/mysql/mysql.go
Outdated
|
|
||
| // CloseDB uses the database handle to close the connection and prevents future queries. | ||
| func CloseDB(db *sql.DB) error { | ||
| err := db.Close() |
There was a problem hiding this comment.
This function seems to be unnecessary.
|
|
||
| // Close closes the database connection and prevents future queries. | ||
| func (m *MetricSet) Close() error { | ||
| err := m.db.Close() |
There was a problem hiding this comment.
This should have a nil check for the db. And errors.Wrap() can safely be called when err is nil and it just returns nil.
if m.db == nil {
return nil
}
return errors.Wrap(m.db.Close(), "failed to close mysql database client")
There was a problem hiding this comment.
The same fix should be added to metricbeat/module/mysql/galera_status/status.go.
There was a problem hiding this comment.
Interesting. Thanks for the feedback.
Question, the loadStatus function in the status.go files does not check the db pointer. Should this check be done too?
func (m *MetricSet) loadStatus(db *sql.DB) (map[string]string, error) {
// Returns the global status, also for versions previous 5.0.2
rows, err := db.Query("SHOW /*!50002 GLOBAL */ STATUS;")
if err != nil {
return nil, err
}
defer rows.Close()
...
There was a problem hiding this comment.
No, this path isn't reachable with a nil db.
| // Close closes the database connection and prevents future queries. | ||
| func (m *MetricSet) Close() error { | ||
| if m.db == nil { | ||
| return errors.New("invalid database handle") |
There was a problem hiding this comment.
It should not be an error to have a nil db. This means that it never fetched or it never successfully initialized the DB client. In any case there's nothing for Close() to do, but it's not an error.
There was a problem hiding this comment.
Ok. I understand, I was missing the scope of work of this function. Now, I understand your initial code feedback. Thank you.
|
Can you please add an entry to the CHANGELOG.next.asciidoc file under the bugs / metricbeat section. |
|
jenkins, test this |
|
Thank you for your patience. |
|
Sure, doing it, thanks @jarrocha for fixing this! |
Fixes elastic#9415 by closing the db handle. (cherry picked from commit f06cf46)
Fixes #9415 by closing the db handle. (cherry picked from commit f06cf46) Co-authored-by: Jaime A <[email protected]>
|
Not a problem. I would like to keep going and grow. Do you have any suggestions? I have taken a look at this one which seems more challenging but you might know better? |
This is a possible fix for issue #9415. Added a function in the mysql file and
a method for the metricset to close the db too.
I wanted to give it a shot since the last update was done more than 1 month ago. After researching the issue and feedback I thought about this possible solution. Is my approach good or do you prefer the metricset method doing all the work without calling to a function in the mysql file?
Let me know if there's a specific place where these functions should be called.
Also,
https://github.com/elastic/beats/blob/824d443bc182a12f30859667da4739beef9c8606/metricbeat/mb/module/wrapper_test.go#L66
is empty. Should it call these new functions for assertion? Just thinking of a possible new commit.