Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

executor: fix two data races in tests #11062

Merged
merged 4 commits into from
Jul 4, 2019
Merged

executor: fix two data races in tests #11062

merged 4 commits into from
Jul 4, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Jul 3, 2019

What problem does this PR solve?

Fix two data races in tests.

What is changed and how it works?

Add atomic store for global variables.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

jackysp added 2 commits July 3, 2019 20:08
@jackysp
fix two races
39e05ed 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
remove useless code
6595215 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp added the sig/execution SIG execution label Jul 3, 2019
@jackysp jackysp requested review from crazycs520 and coocood July 3, 2019 12:23
crazycs520
crazycs520 reviewed Jul 3, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crazycs520
Copy link
Contributor

/run-all-tests

coocood
coocood reviewed Jul 3, 2019
View reviewed changes
executor/executor_test.go
@@ -4097,7 +4109,7 @@ func (s *testSuite) TestOOMPanicAction(c *C) {
}
tk.Se.SetSessionManager(sm)
s.domain.ExpensiveQueryHandle().SetSessionManager(sm)
config.GetGlobalConfig().OOMAction = config.OOMActionCancel
setOOMAction(config.OOMActionCancel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to set it back?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jackysp
set oom action back
be7ac9e 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Jul 3, 2019

/run-all-tests

@coocood
Copy link
Member

coocood commented Jul 3, 2019

LGTM

@jackysp
Copy link
Member Author

jackysp commented Jul 3, 2019 •
edited
Loading

go test -vet=off -p 5 -timeout 10m -race -count 1 github.com/pingcap/tidb/ddl reached the test timeout limit and was killed, but I did not touch ddl package...
full log:

+ date

Wed Jul  3 20:30:01 CST 2019

+ set +e

+ killall -9 -r tidb-server

tidb-server: no process found

+ killall -9 -r tikv-server

tikv-server: no process found

+ killall -9 -r pd-server

pd-server: no process found

+ rm -rf /tmp/tidb

+ set -e

+ date

Wed Jul  3 20:30:01 CST 2019

+ mkdir -p /go/pkg/mod

+ mkdir -p /home/jenkins/workspace/tidb_ghpr_unit_test/go/pkg

+ ln -sf /go/pkg/mod /home/jenkins/workspace/tidb_ghpr_unit_test/go/pkg/mod

+ export log_level=debug

+ log_level=debug

+ date

Wed Jul  3 20:30:01 CST 2019

++ cat packages_race_8

+ GOPATH=/home/jenkins/workspace/tidb_ghpr_unit_test/go

+ CGO_ENABLED=1

+ GO111MODULE=on

+ go test -vet=off -p 5 -timeout 10m -race -count 1 github.com/pingcap/tidb/ddl

Sending interrupt signal to process

Killing processes

sh: line 1: 19082 Terminated              JENKINS_SERVER_COOKIE=$jsc '/bin/sh' -xe '/home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb@tmp/durable-ee99cd4e/script.sh' > '/home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb@tmp/durable-ee99cd4e/jenkins-log.txt' 2>&1

kill finished with exit code 0

script returned exit code 143

@jackysp
Copy link
Member Author

jackysp commented Jul 3, 2019

/run-unit-test

@coocood coocood mentioned this pull request Jul 4, 2019
Closed
@tiancaiamao
Copy link
Contributor

/run-unit-test

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 4, 2019
@tiancaiamao
Copy link
Contributor

/run-unit-test

@codecov
Copy link

codecov bot commented Jul 4, 2019

Codecov Report

Merging #11062 into master will decrease coverage by 0.2558%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11062        +/-   ##
================================================
- Coverage   81.2145%   80.9586%   -0.2559%     
================================================
  Files           419        419                
  Lines         90682      89332      -1350     
================================================
- Hits          73647      72322      -1325     
+ Misses        11778      11772         -6     
+ Partials       5257       5238        -19

@tiancaiamao tiancaiamao merged commit 027851f into pingcap:master Jul 4, 2019
@SunRunAway SunRunAway mentioned this pull request Jul 8, 2019
Merged
@jackysp jackysp deleted the fix_race branch February 27, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@crazycs520 crazycs520 crazycs520 left review comments

@tiancaiamao tiancaiamao tiancaiamao approved these changes

Assignees
No one assigned
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackysp @crazycs520 @coocood @tiancaiamao