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

chore: populate the missing kafka versions #2035

Merged
merged 4 commits into from
Sep 22, 2021
Merged

chore: populate the missing kafka versions #2035

merged 4 commits into from
Sep 22, 2021

Conversation

dnwe
Copy link
Collaborator

@dnwe dnwe commented Sep 20, 2021

As discussed under #2034, should simplify linkedin/Burrow usage too.

@dnwe
chore: populate the missing kafka versions
1cdea0a 
As discussed under #2034, should simplify linkedin/Burrow usage too.
@dnwe dnwe requested a review from bai as a code owner September 20, 2021 23:44
@bai
Copy link
Contributor

bai commented Sep 21, 2021

Off, tests are so flaky.

@dnwe
Copy link
Collaborator Author

dnwe commented Sep 21, 2021

flakey, but differently flakey — which is interesting

  1. *** Test killed with quit: ran too long (7m0s) for TestFuncProducingIdempotentWithBrokerFailure

  2. TestClientController data race

2021-09-21T05:06:59.9357821Z WARNING: DATA RACE
2021-09-21T05:06:59.9358284Z Write at 0x00c0000ff4a8 by goroutine 152:
2021-09-21T05:06:59.9358971Z   github.com/Shopify/sarama.TestClientController()
2021-09-21T05:06:59.9359771Z       /home/runner/work/sarama/sarama/client_test.go:705 +0x667
2021-09-21T05:06:59.9360297Z   testing.tRunner()
2021-09-21T05:06:59.9361359Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1259 +0x22f
2021-09-21T05:06:59.9363238Z   testing.(*T).Run·dwrap·21()
2021-09-21T05:06:59.9364147Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1306 +0x47
2021-09-21T05:06:59.9364850Z 
2021-09-21T05:06:59.9365424Z Previous read at 0x00c0000ff4a8 by goroutine 79:
2021-09-21T05:06:59.9366231Z   runtime.racereadrange()
2021-09-21T05:06:59.9366841Z       <autogenerated>:1 +0x1b
2021-09-21T05:06:59.9367444Z   github.com/Shopify/sarama.(*Broker).Open.func1()
2021-09-21T05:06:59.9368159Z       /home/runner/work/sarama/sarama/broker.go:228 +0x110b
2021-09-21T05:06:59.9368955Z   github.com/Shopify/sarama.withRecover()
2021-09-21T05:06:59.9369712Z       /home/runner/work/sarama/sarama/utils.go:43 +0x74
2021-09-21T05:06:59.9370593Z   github.com/Shopify/sarama.(*Broker).Open·dwrap·20()
2021-09-21T05:06:59.9371371Z       /home/runner/work/sarama/sarama/broker.go:155 +0x39

@dnwe
fix: prevent data race on conf.Version
aa7ce85 
Move conf.Version check out of background goroutine to prevent
read+write race in test.

Wrap TestClientController subtests in t.Run(...) whilst in here.
@dnwe
chore: bump up the test timeout to 10m
01c7c43 
We seem to be hitting `*** Test killed: ran too long (7m0s)` for the
functional test(s), and whilst I do see a large number of hung
goroutines in the backtrace, I can also see that it is mid teardown of
the docker containers, so it seems to have finished the actual testing
and might just be taking longer than usual to `docker stop` the kafka
containers.
@dnwe
Copy link
Collaborator Author

dnwe commented Sep 22, 2021

Hmm another data race:

2021-09-22T13:24:31.3603927Z ==================
2021-09-22T13:24:31.3604287Z WARNING: DATA RACE
2021-09-22T13:24:31.3604758Z Write at 0x00c0003e4b10 by goroutine 108:
2021-09-22T13:24:31.3605308Z   runtime.mapassign_faststr()
2021-09-22T13:24:31.3606134Z       /opt/hostedtoolcache/go/1.17.1/x64/src/runtime/map_faststr.go:202 +0x0
2021-09-22T13:24:31.3606903Z   github.com/Shopify/sarama.TestDeleteOffset()
2021-09-22T13:24:31.3608024Z       /home/runner/work/sarama/sarama/admin_test.go:1366 +0x5de
2021-09-22T13:24:31.3608577Z   testing.tRunner()
2021-09-22T13:24:31.3609220Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1259 +0x22f
2021-09-22T13:24:31.3611336Z   testing.(*T).Run·dwrap·21()
2021-09-22T13:24:31.3612084Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1306 +0x47
2021-09-22T13:24:31.3612554Z 
2021-09-22T13:24:31.3613033Z Previous read at 0x00c0003e4b10 by goroutine 85:
2021-09-22T13:24:31.3613668Z   runtime.mapaccess1_faststr()
2021-09-22T13:24:31.3614431Z       /opt/hostedtoolcache/go/1.17.1/x64/src/runtime/map_faststr.go:12 +0x0
2021-09-22T13:24:31.3615331Z   github.com/Shopify/sarama.(*MockBroker).SetHandlerByMap.func1()
2021-09-22T13:24:31.3616199Z       /home/runner/work/sarama/sarama/mockbroker.go:88 +0xc4
2021-09-22T13:24:31.3616974Z   github.com/Shopify/sarama.(*MockBroker).handleRequests()
2021-09-22T13:24:31.3618158Z       /home/runner/work/sarama/sarama/mockbroker.go:261 +0x941
2021-09-22T13:24:31.3619146Z   github.com/Shopify/sarama.(*MockBroker).serverLoop·dwrap·84()
2021-09-22T13:24:31.3619903Z       /home/runner/work/sarama/sarama/mockbroker.go:172 +0x71
2021-09-22T13:24:31.3620292Z 
2021-09-22T13:24:31.3620875Z Goroutine 108 (running) created at:
2021-09-22T13:24:31.3621354Z   testing.(*T).Run()
2021-09-22T13:24:31.3621970Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1306 +0x726
2021-09-22T13:24:31.3622649Z   testing.runTests.func1()
2021-09-22T13:24:31.3623361Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1598 +0x99
2021-09-22T13:24:31.3624108Z   testing.tRunner()
2021-09-22T13:24:31.3624919Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1259 +0x22f
2021-09-22T13:24:31.3625531Z   testing.runTests()
2021-09-22T13:24:31.3626176Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1596 +0x7ca
2021-09-22T13:24:31.3626748Z   testing.(*M).Run()
2021-09-22T13:24:31.3627521Z       /opt/hostedtoolcache/go/1.17.1/x64/src/testing/testing.go:1504 +0x9d1
2021-09-22T13:24:31.3628174Z   github.com/Shopify/sarama.testMain()
2021-09-22T13:24:31.3628823Z       /home/runner/work/sarama/sarama/functional_test.go:94 +0x2c4
2021-09-22T13:24:31.3629450Z   github.com/Shopify/sarama.TestMain()
2021-09-22T13:24:31.3630117Z       /home/runner/work/sarama/sarama/functional_test.go:67 +0x2e
2021-09-22T13:24:31.3630615Z   main.main()
2021-09-22T13:24:31.3631029Z       _testmain.go:1159 +0x1ee
2021-09-22T13:24:31.3631310Z 
2021-09-22T13:24:31.3631705Z Goroutine 85 (running) created at:
2021-09-22T13:24:31.3632290Z   github.com/Shopify/sarama.(*MockBroker).serverLoop()
2021-09-22T13:24:31.3632995Z       /home/runner/work/sarama/sarama/mockbroker.go:172 +0x1d2
2021-09-22T13:24:31.3634019Z   github.com/Shopify/sarama.NewMockBrokerListener·dwrap·87()
2021-09-22T13:24:31.3635103Z       /home/runner/work/sarama/sarama/mockbroker.go:415 +0x39

It seems like Go has recently improved its race detector to catch more issues or something?

@dnwe
fix(test): use copy in mockbroker SetHandlersByMap
8a2c8ba 
Clone the given handlerMap (to prevent race conditions on modification),
and require further calls to SetHandlerByMap if the handlers need
updating mid test.
@dnwe dnwe merged commit 461b931 into main Sep 22, 2021
@dnwe dnwe deleted the dnwe/add-missing-version-constants branch September 22, 2021 14:03
@dnwe
Copy link
Collaborator Author

dnwe commented Sep 22, 2021

😅 passed, merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bai bai bai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dnwe @bai