executor: initialize expensive query handler on domain creation #21340

hanlins · 2020-11-27T02:25:30Z

What problem does this PR solve?

Issue Number: close #18401

Problem Summary:

What is changed and how it works?

What's Changed:
Merge the expensive query handler initialization logic into the NewDomain function.

How it Works:

Originally the query execution handler is not initialized, thus LogOnQueryExceedMemQuota is a closure that accesses nil pointer. Now initialize query execution handler on Domain creation, so it won't access the nil pointer during bootstrap.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Manual test (add detailed scripts or steps below)

Side effects

Performance regression
NA
Breaking backward compatibility
NA

Release note

Fix expensive query handler initialization during bootstrap

CLAassistant · 2020-11-27T02:25:35Z

All committers have signed the CLA.

sre-bot · 2020-11-27T02:25:46Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

hanlins · 2020-11-27T02:30:01Z

/rebuild

sre-bot · 2020-11-27T02:30:08Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

hanlins · 2020-11-27T02:34:13Z

/rebuild

xiongjiwei · 2020-11-27T08:15:15Z

session/bootstrap_test.go

@@ -388,19 +388,6 @@ func (s *testBootstrapSuite) TestOldPasswordUpgrade(c *C) {
 	c.Assert(newpwd, Equals, "*0D3CED9BEC10A777AEC23CCC353A8C08A633045E")
 }

-func (s *testBootstrapSuite) TestBootstrapInitExpensiveQueryHandle(c *C) {


why remove this test

The InitExpensiveQueryHandle function is removed so I don't see the point testing against it. But maybe instead of removing the test, I could just remove InitExpensiveQueryHandle invocation and just check the existence of that query handler to avoid regression. Let me add that.

hanlins · 2020-12-02T05:06:17Z

/assign xiongjiwei

xiongjiwei · 2020-12-03T09:37:19Z

@XuHuaiyu PTAL .

wshwsh12

LGTM

xiongjiwei · 2021-01-05T06:23:23Z

/run-all-tests

hanlins · 2021-01-05T08:13:03Z

/run-all-tests

hanlins · 2021-01-27T06:41:59Z

Hi @xiongjiwei @XuHuaiyu PTAL, thanks

xhebox

/lgtm

ti-chi-bot · 2021-03-11T05:31:38Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

xhebox · 2021-03-11T05:31:46Z

/merge

ti-chi-bot · 2021-03-11T05:31:49Z

This pull request has been accepted and is ready to merge.

Commit hash: 661d80eed0aa5034e2fef1a3f687759b7c96cea1

Signed-off-by: Hanlin Shi <[email protected]>

As query handler is being initialized during domain init, not need for duplicated handler initialization. Signed-off-by: Hanlin Shi <[email protected]>

Added expensive query handler existence check back to avoid potential regression. Signed-off-by: Hanlin Shi <[email protected]>

xhebox · 2021-03-11T05:43:18Z

Are you changing anything?

hanlins · 2021-03-11T05:45:04Z

Are you changing anything?

No change just rebase.

xhebox · 2021-03-11T05:45:21Z

/merge

ti-chi-bot · 2021-03-11T05:45:23Z

This pull request has been accepted and is ready to merge.

Commit hash: e300469

xhebox · 2021-03-11T05:48:00Z

No change just rebase.

Bot will automatically merge the master branch. And the merge of PR will squash/rebase the commit history into the linear one. So you don't need to rebase unless there is a conflict or so.

hanlins · 2021-03-11T05:51:14Z

Bot will automatically merge the master branch. And the merge of PR will squash/rebase the commit history into the linear one. So you don't need to rebase unless there is a conflict or so.

Got that, thanks!

hanlins · 2021-03-11T07:41:04Z

/run-all-tests

Rustin170506 · 2021-03-11T07:41:30Z

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Rustin170506 · 2021-03-11T07:43:02Z

You can only trigger the failed test by /run-test-name.

ti-chi-bot · 2021-03-11T07:55:11Z

@hanlins: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Nov 27, 2020

github-actions bot added the sig/sql-infra SIG: SQL Infra label Nov 27, 2020

xiongjiwei reviewed Nov 27, 2020

View reviewed changes

hanlins requested a review from xiongjiwei November 27, 2020 08:38

ichn-hu mentioned this pull request Nov 27, 2020

Welcome to contribute #20804

Closed

ti-srebot assigned xiongjiwei Dec 2, 2020

wshwsh12 reviewed Dec 23, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 23, 2020

wshwsh12 requested a review from XuHuaiyu December 23, 2020 07:58

xiongjiwei removed their assignment Dec 23, 2020

github-actions bot added the sig/DDL label Dec 24, 2020

github-actions bot added the sig/infra label Feb 22, 2021

ti-chi-bot removed sig/infra labels Feb 22, 2021

xhebox approved these changes Mar 11, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 11, 2021

ti-chi-bot added status/can-merge Indicates a PR has been approved by a committer. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 11, 2021

hanlins added 3 commits March 10, 2021 21:41

Initialize expensive query handler during domain init

545712e

Signed-off-by: Hanlin Shi <[email protected]>

Remove reference of InitExpensiveQueryHandle

dfcf86f

As query handler is being initialized during domain init, not need for duplicated handler initialization. Signed-off-by: Hanlin Shi <[email protected]>

Add TestBootstrapInitExpensiveQueryHandle back

e300469

Added expensive query handler existence check back to avoid potential regression. Signed-off-by: Hanlin Shi <[email protected]>

hanlins force-pushed the fix/18401/query-handler-init branch from 600971a to e300469 Compare March 11, 2021 05:41

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 11, 2021

hanlins requested a review from xhebox March 11, 2021 05:42

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 11, 2021

ti-chi-bot added 3 commits March 11, 2021 14:15

Merge branch 'master' into fix/18401/query-handler-init

20ec489

Merge branch 'master' into fix/18401/query-handler-init

48d3237

Merge branch 'master' into fix/18401/query-handler-init

6baa3e5

Merge branch 'master' into fix/18401/query-handler-init

f3ac214

ti-chi-bot merged commit 67e4528 into pingcap:master Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: initialize expensive query handler on domain creation #21340

executor: initialize expensive query handler on domain creation #21340

hanlins commented Nov 27, 2020 •

edited

Loading

CLAassistant commented Nov 27, 2020 •

edited

Loading

sre-bot commented Nov 27, 2020

hanlins commented Nov 27, 2020

sre-bot commented Nov 27, 2020

hanlins commented Nov 27, 2020

xiongjiwei Nov 27, 2020

hanlins Nov 27, 2020

hanlins Nov 27, 2020

hanlins commented Dec 2, 2020

xiongjiwei commented Dec 3, 2020

wshwsh12 left a comment

xiongjiwei commented Jan 5, 2021

hanlins commented Jan 5, 2021

hanlins commented Jan 27, 2021

xhebox left a comment

ti-chi-bot commented Mar 11, 2021

xhebox commented Mar 11, 2021

ti-chi-bot commented Mar 11, 2021

xhebox commented Mar 11, 2021

hanlins commented Mar 11, 2021

xhebox commented Mar 11, 2021

ti-chi-bot commented Mar 11, 2021

xhebox commented Mar 11, 2021

hanlins commented Mar 11, 2021

hanlins commented Mar 11, 2021

Rustin170506 commented Mar 11, 2021

Rustin170506 commented Mar 11, 2021 •

edited

Loading

ti-chi-bot commented Mar 11, 2021

executor: initialize expensive query handler on domain creation #21340

executor: initialize expensive query handler on domain creation #21340

Conversation

hanlins commented Nov 27, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

CLAassistant commented Nov 27, 2020 • edited Loading

sre-bot commented Nov 27, 2020

hanlins commented Nov 27, 2020

sre-bot commented Nov 27, 2020

hanlins commented Nov 27, 2020

xiongjiwei Nov 27, 2020

Choose a reason for hiding this comment

hanlins Nov 27, 2020

Choose a reason for hiding this comment

hanlins Nov 27, 2020

Choose a reason for hiding this comment

hanlins commented Dec 2, 2020

xiongjiwei commented Dec 3, 2020

wshwsh12 left a comment

Choose a reason for hiding this comment

xiongjiwei commented Jan 5, 2021

hanlins commented Jan 5, 2021

hanlins commented Jan 27, 2021

xhebox left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 11, 2021

xhebox commented Mar 11, 2021

ti-chi-bot commented Mar 11, 2021

xhebox commented Mar 11, 2021

hanlins commented Mar 11, 2021

xhebox commented Mar 11, 2021

ti-chi-bot commented Mar 11, 2021

xhebox commented Mar 11, 2021

hanlins commented Mar 11, 2021

hanlins commented Mar 11, 2021

Rustin170506 commented Mar 11, 2021

Rustin170506 commented Mar 11, 2021 • edited Loading

ti-chi-bot commented Mar 11, 2021

hanlins commented Nov 27, 2020 •

edited

Loading

CLAassistant commented Nov 27, 2020 •

edited

Loading

Rustin170506 commented Mar 11, 2021 •

edited

Loading