fix: close mysql statement and rows resources when sql exec end #1720

chenwenjun-github · 2021-10-22T06:56:20Z

What this PR does / why we need it:
close mysql statement and rows resources when sql exec end, reslove mysql statement leak

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1719

Checklist:

Docs included if any changes are user facing

google-cla · 2021-10-22T06:56:22Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

aws-kf-ci-bot · 2021-10-22T06:56:33Z

Hi @chenwenjun-github. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chenwenjun-github · 2021-10-22T06:59:47Z

@googlebot I fixed it.

google-cla · 2021-10-22T06:59:50Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

johnugeorge · 2021-10-22T09:12:23Z

/ok-to-test

gaocegege

LGTM

/assign @andreyvelich

johnugeorge · 2021-10-22T09:21:33Z

@chenwenjun-github Thanks for creating this.
Can you fix CLA issue? We can merge this when CI passes

coveralls · 2021-10-22T10:20:57Z

Coverage increased (+0.2%) to 73.723% when pulling 6fe941a on chenwenjun-github:close_mysql_stmt into 195db29 on kubeflow:master.

midhun1998 · 2021-10-22T10:56:16Z

pkg/db/v1beta1/mysql/mysql.go

@@ -132,6 +132,9 @@ func (d *dbConn) RegisterObservationLog(trialName string, observationLog *v1beta
 		return fmt.Errorf("Pepare SQL statement failed: %v", err)


Thank you for the fix @chenwenjun-github ! Could you please also correct the typo here? (Prepare)
Thanks!

andreyvelich

Thank you for debugging this @chenwenjun-github!
I hope this will resolve issue: #1651.

andreyvelich · 2021-10-25T23:10:00Z

pkg/db/v1beta1/mysql/mysql.go

@@ -179,6 +182,8 @@ func (d *dbConn) GetObservationLog(trialName string, metricName string, startTim
 	result := &v1beta1.ObservationLog{
 		MetricLogs: []*v1beta1.MetricLog{},
 	}
+	// Close the rows
+	defer rows.Close()


Please can you move this right after the error check (line number 182), similar to stmt.Close().

…ub/katib into close_mysql_stmt update commit email

google-oss-robot · 2021-10-26T03:26:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chenwenjun-github
To complete the pull request process, please ask for approval from andreyvelich after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-cla · 2021-10-26T03:26:35Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

chenwenjun-github · 2021-10-27T07:23:17Z

@googlebot I fixed it.

chenwenjun-github · 2021-10-27T07:40:36Z

@chenwenjun-github Thanks for creating this. Can you fix CLA issue? We can merge this when CI passes

done

andreyvelich · 2021-11-02T13:27:35Z

Thank you for updating this @chenwenjun-github!
/lgtm
/assign @gaocegege @johnugeorge

johnugeorge · 2021-11-02T19:16:46Z

/approve

google-oss-prow · 2021-11-02T19:17:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenwenjun-github, johnugeorge

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [johnugeorge]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fix: close mysql statement and rows resources when sql exec end

b278ce8

aws-kf-ci-bot added the needs-ok-to-test label Oct 22, 2021

google-oss-robot added the size/XS label Oct 22, 2021

google-oss-robot requested review from andreyvelich, gaocegege and sperlingxx October 22, 2021 06:56

aws-kf-ci-bot added ok-to-test and removed needs-ok-to-test labels Oct 22, 2021

gaocegege reviewed Oct 22, 2021

View reviewed changes

google-oss-robot assigned andreyvelich Oct 22, 2021

johnugeorge mentioned this pull request Oct 22, 2021

Katib MySQL reaching max_prepared_stmt_count limit #1651

Closed

midhun1998 reviewed Oct 22, 2021

View reviewed changes

andreyvelich reviewed Oct 25, 2021

View reviewed changes

chenwenjun-github added 2 commits October 26, 2021 11:24

fix: close mysql statement and rows resources when sql exec end

b980da3

Merge branch 'close_mysql_stmt' of https://github.com/chenwenjun-gith…

985c7b2

…ub/katib into close_mysql_stmt update commit email

chenwenjun01 added 2 commits October 27, 2021 15:36

style: move code to other place

0671087

style: correct the typo(Prepare)

6fe941a

google-oss-prow bot assigned gaocegege and johnugeorge Nov 2, 2021

google-oss-prow bot added the lgtm label Nov 2, 2021

google-oss-prow bot added the approved label Nov 2, 2021

google-oss-robot merged commit bfe4527 into kubeflow:master Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: close mysql statement and rows resources when sql exec end #1720

fix: close mysql statement and rows resources when sql exec end #1720

chenwenjun-github commented Oct 22, 2021

google-cla bot commented Oct 22, 2021

aws-kf-ci-bot commented Oct 22, 2021

chenwenjun-github commented Oct 22, 2021

google-cla bot commented Oct 22, 2021

johnugeorge commented Oct 22, 2021

gaocegege left a comment

johnugeorge commented Oct 22, 2021

coveralls commented Oct 22, 2021 •

edited

Loading

midhun1998 Oct 22, 2021

chenwenjun-github Oct 27, 2021

andreyvelich left a comment

andreyvelich Oct 25, 2021

chenwenjun-github Oct 27, 2021

google-oss-robot commented Oct 26, 2021

google-cla bot commented Oct 26, 2021

chenwenjun-github commented Oct 27, 2021

chenwenjun-github commented Oct 27, 2021

andreyvelich commented Nov 2, 2021

johnugeorge commented Nov 2, 2021

google-oss-prow bot commented Nov 2, 2021

		@@ -132,6 +132,9 @@ func (d dbConn) RegisterObservationLog(trialName string, observationLog v1beta
		return fmt.Errorf("Pepare SQL statement failed: %v", err)

fix: close mysql statement and rows resources when sql exec end #1720

fix: close mysql statement and rows resources when sql exec end #1720

Conversation

chenwenjun-github commented Oct 22, 2021

google-cla bot commented Oct 22, 2021

aws-kf-ci-bot commented Oct 22, 2021

chenwenjun-github commented Oct 22, 2021

google-cla bot commented Oct 22, 2021

johnugeorge commented Oct 22, 2021

gaocegege left a comment

Choose a reason for hiding this comment

johnugeorge commented Oct 22, 2021

coveralls commented Oct 22, 2021 • edited Loading

midhun1998 Oct 22, 2021

Choose a reason for hiding this comment

chenwenjun-github Oct 27, 2021

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

andreyvelich Oct 25, 2021

Choose a reason for hiding this comment

chenwenjun-github Oct 27, 2021

Choose a reason for hiding this comment

google-oss-robot commented Oct 26, 2021

google-cla bot commented Oct 26, 2021

chenwenjun-github commented Oct 27, 2021

chenwenjun-github commented Oct 27, 2021

andreyvelich commented Nov 2, 2021

johnugeorge commented Nov 2, 2021

google-oss-prow bot commented Nov 2, 2021

coveralls commented Oct 22, 2021 •

edited

Loading