session: migrate test-infra to testify for tidb_test.go and bootstrap_test.go by tisonkun · Pull Request #28555 · pingcap/tidb

tisonkun · 2021-10-07T08:44:05Z

This closes #28322 .
This closes #28329 .

Release note

None

…_test.go This closes pingcap#28322 . This closes pingcap#28329 . Signed-off-by: tison <wander4096@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>

ti-chi-bot · 2021-10-07T08:44:07Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
morgo

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.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

tisonkun · 2021-10-07T08:55:41Z

/cc @lance6716 @morgo

@lance6716 this PR changes the failpoint you introduced in #18714 to avoid goroutine leak. Please review.

tisonkun · 2021-10-07T08:56:25Z

ddl/ddl.go

 	for {
 		failpoint.Inject("storeCloseInLoop", func(_ failpoint.Value) {
-			d.cancel()
+			_ = d.Stop()


@lance6716 at this line only, so that we don't just cancel the context but trigger a graceful shutdown. I'm unsure whether or not it is the original failpoint want to do. In another way, we can ignore the leak.

I agree with this change.

The original failpoint is to imitate that the TiDB-as-a-library is being closed when handling a DDL, to check if the goroutine is stuck inside this for-loop. My original test code is #18714 , but after some iterations it becomes current invasive failpoint 😂

I'll review rest of code tomorrow

tisonkun · 2021-10-07T08:59:41Z

session/bootstrap_serial_test.go

+	oomAction := config.GetGlobalConfig().OOMAction
+	defer func() {
+		config.UpdateGlobal(func(conf *config.Config) {
+			conf.OOMAction = oomAction
+		})
+	}()
+	ctx := context.Background()


Add this store and restore for

TestUpgrade

TestIssue17979_1

TestIssue20900_2

Otherwise TestIssue17979_2 failed as TestUpgrade set the default oomAction to "log". Previously we don't meet this issue possibly due to different tests executed order.

names like TestIssue17979_1 is not good right? how about give them proper name and add link in comments?

tisonkun · 2021-10-07T09:00:26Z

session/bootstrap_serial_test.go

+	}
+
+	for _, ca := range cases {
+		func() {


wrap in a function so that defer works properly.

tisonkun · 2021-10-07T09:00:56Z

session/bootstrap_test.go

-	s.dbName = "test_bootstrap"
-	s.dbNameBootstrap = "test_main_db_bootstrap"


we don't actually make use of them

tisonkun · 2021-10-07T09:01:45Z

session/index_usage_sync_lease_serial_test.go

+	SetIndexUsageSyncLease(1)
+	defer SetIndexUsageSyncLease(0)


Changes global variable and thus run in serial.

tisonkun · 2021-10-07T09:02:38Z

session/main_test.go

+	require.NoError(t, err)
+	return se


se.Auth is for TestForIssue23387 only.

tisonkun · 2021-10-07T09:03:50Z

session/tidb_test.go

-func removeStore(c *C, dbPath string) {
-	os.RemoveAll(dbPath)
-}


Not really in use.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2021-10-07T09:32:36Z

/run-check_dev_2

tisonkun · 2021-10-07T09:55:58Z

/run-check_dev_2

morgo

Looking good

session/bootstrap_serial_test.go

morgo · 2021-10-08T15:08:34Z

session/bootstrap_serial_test.go

+		if v.Scope != variable.ScopeSession {
+			count++
+		}


This is a bit mask of options, testing for ScopeSession might be buggy if we introduce any other scopes like ScopeInstance. Using helper funcs is better suited:

tidb/sessionctx/variable/sysvar.go

Lines 234 to 247 in d458059

// HasNoneScope returns true if the scope for the sysVar is None.

func (sv *SysVar) HasNoneScope() bool {

return sv.Scope == ScopeNone

}

// HasSessionScope returns true if the scope for the sysVar includes session.

func (sv *SysVar) HasSessionScope() bool {

return sv.Scope&ScopeSession != 0

}

// HasGlobalScope returns true if the scope for the sysVar includes global.

func (sv *SysVar) HasGlobalScope() bool {

return sv.Scope&ScopeGlobal != 0

}

I did not realize it, but ScopeNone variables are also persisted to the table (i.e. != ScopeSession is both Global and None). This looks to be intentional because system_time_zone needs to be persisted to the cluster. There might actually be bugs here because a Get on a ScopeNone returns sv.Value and doesn't check the persisted value:

tidb/sessionctx/variable/varsutil.go

Lines 172 to 182 in d458059

// GetSessionOrGlobalSystemVar gets a system variable.

// If it is a session only variable, use the default value defined in code.

// Returns error if there is no such variable.

func GetSessionOrGlobalSystemVar(s *SessionVars, name string) (string, error) {

sv := GetSysVar(name)

if sv == nil {

return "", ErrUnknownSystemVar.GenWithStackByArgs(name)

}

if sv.HasNoneScope() {

return sv.Value, nil

}

I did some additional research, and all the ScopeNone variables look to be safe to non-persist in the system table. They are usually overwritten by each TiDB server during startup.

I've forked this to #28667

session/bootstrap_serial_test.go

Co-authored-by: Morgan Tocker <tocker@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2021-10-09T08:42:43Z

@morgo thanks for your review. Your comments are addressed.

For #28667 , let's keep track on the issue.

morgo · 2021-10-09T18:35:11Z

/run-check_dev_2

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2021-10-11T01:56:33Z

@morgo CI fixed. Please give this PR one more round review.

morgo · 2021-10-11T02:16:26Z

/merge

ti-chi-bot · 2021-10-11T02:16:29Z

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

Details

Commit hash: ac6a0db

tiancaiamao · 2021-10-11T05:51:55Z

session/main_test.go

+		conf.TiKVClient.AsyncCommit.SafeWindow = 0
+		conf.TiKVClient.AsyncCommit.AllowedClockDrift = 0
+	})
+	tikv.EnableFailpoints()


This change cause a performance regression.
In the old code, EnableFailpoints only works on the normal test cases.
After the change, EnableFailpoints will affect the benchmark tests.

When the the failpoint is not found in the benchmark test code, it trigger this code path

See the chart here http://www.zenlife.tk:18081/

@tiancaiamao is there a way configuring tests and benchmarks separately? Previous we turn on failpoints in test, but it is a global state change. I guess we don't observe "regression" then because it occasionally run benchmark at first?

@disksing IIUC we discuss about tikv failpoints regressions ever, could you also take a look at this?

@tiancaiamao emmm, it is because we already run benchmark & tests separately, but TestMain affects either.

Is it possible to check if it is running bench or test?

@tiancaiamao hacking and found several candidates to address this regression, please check #28709

Is it possible to check if it is running bench or test?

@disksing I share the same preference with you and here is one possible solution #28709 . Please check.

I have to say that "check if it is running bench or test" isn't supported out-of-the-box by Golang, but we MAY achieve it manually.

tisonkun added 2 commits October 7, 2021 15:43

session: migrate test-infra to testify for tidb_test.go and bootstrap…

2f3da26

…_test.go This closes pingcap#28322 . This closes pingcap#28329 . Signed-off-by: tison <wander4096@gmail.com>

fix leaks

0643d2d

Signed-off-by: tison <wander4096@gmail.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 7, 2021

Merge branch 'master' into session-testify

85dcdbc

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2021

ti-chi-bot requested review from lance6716 and morgo October 7, 2021 08:55

tisonkun commented Oct 7, 2021

View reviewed changes

ignore unknown leaks

38ecc25

Signed-off-by: tison <wander4096@gmail.com>

lance6716 approved these changes Oct 8, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 8, 2021

morgo reviewed Oct 8, 2021

View reviewed changes

morgo mentioned this pull request Oct 8, 2021

tidb-server should not persist ScopeNone sysvars during bootstrap #28667

Closed

feitian124 reviewed Oct 9, 2021

View reviewed changes

session/bootstrap_serial_test.go Show resolved Hide resolved

tisonkun and others added 2 commits October 9, 2021 13:57

Update session/bootstrap_serial_test.go

70c6706

Co-authored-by: Morgan Tocker <tocker@gmail.com>

test mysql.global_grants

098bc9c

Signed-off-by: tison <wander4096@gmail.com>

tisonkun added 2 commits October 10, 2021 17:45

Merge branch 'master' into session-testify

82f1273

ignore unknown leak

ac6a0db

Signed-off-by: tison <wander4096@gmail.com>

morgo self-requested a review October 11, 2021 02:16

morgo approved these changes Oct 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 Oct 11, 2021

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

ti-chi-bot merged commit 2f9d591 into pingcap:master Oct 11, 2021

tisonkun deleted the session-testify branch October 11, 2021 02:24

tiancaiamao reviewed Oct 11, 2021

View reviewed changes

tisonkun mentioned this pull request Oct 11, 2021

Avoid testing setup for benchmarks #28709

Closed

CabinfeverB mentioned this pull request Aug 24, 2023

test: remove useless code #46408

Merged

12 tasks

		s.dbName = "test_bootstrap"
		s.dbNameBootstrap = "test_main_db_bootstrap"

	// HasNoneScope returns true if the scope for the sysVar is None.
	func (sv *SysVar) HasNoneScope() bool {
	return sv.Scope == ScopeNone
	}

	// HasSessionScope returns true if the scope for the sysVar includes session.
	func (sv *SysVar) HasSessionScope() bool {
	return sv.Scope&ScopeSession != 0
	}

	// HasGlobalScope returns true if the scope for the sysVar includes global.
	func (sv *SysVar) HasGlobalScope() bool {
	return sv.Scope&ScopeGlobal != 0
	}

	// GetSessionOrGlobalSystemVar gets a system variable.
	// If it is a session only variable, use the default value defined in code.
	// Returns error if there is no such variable.
	func GetSessionOrGlobalSystemVar(s *SessionVars, name string) (string, error) {
	sv := GetSysVar(name)
	if sv == nil {
	return "", ErrUnknownSystemVar.GenWithStackByArgs(name)
	}
	if sv.HasNoneScope() {
	return sv.Value, nil
	}

Conversation

tisonkun commented Oct 7, 2021

Release note

Uh oh!

ti-chi-bot commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tisonkun commented Oct 7, 2021

Uh oh!

tisonkun Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Oct 7, 2021

Uh oh!

tisonkun commented Oct 7, 2021

Uh oh!

morgo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tisonkun commented Oct 9, 2021

Uh oh!

morgo commented Oct 9, 2021

Uh oh!

tisonkun commented Oct 11, 2021

Uh oh!

morgo commented Oct 11, 2021

Uh oh!

ti-chi-bot commented Oct 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiancaiamao Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ti-chi-bot commented Oct 7, 2021 •

edited

Loading

tisonkun Oct 7, 2021 •

edited

Loading

tisonkun Oct 7, 2021 •

edited

Loading

tiancaiamao Oct 11, 2021 •

edited

Loading