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

sql: unskip TestSplitAt #30623

Merged
merged 2 commits into from
Sep 25, 2018
Merged

sql: unskip TestSplitAt #30623

merged 2 commits into from
Sep 25, 2018

Conversation

tbg
Copy link
Member

@tbg tbg commented Sep 25, 2018

The failure was fixed in #29324.

Closes #29169.

Release note: None

@tbg
sql: unskip TestSplitAt
e3a93f2 
The failure was fixed in cockroachdb#29324.

Closes cockroachdb#29169.

Release note: None
@tbg tbg requested review from a team September 25, 2018 11:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member Author

tbg commented Sep 25, 2018

Reliably hits a data race:

WARNING: DATA RACE
Read at 0x00c421ade548 by goroutine 43:
  github.com/cockroachdb/cockroach/pkg/storage.(*contentionQueue).add()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/storage/intent_resolver.go:249 +0x1d47
  github.com/cockroachdb/cockroach/pkg/storage.(*intentResolver).processWriteIntentError()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/storage/intent_resolver.go:420 +0x365
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).Send()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:3252 +0xfd3
  github.com/cockroachdb/cockroach/pkg/storage.(*Stores).Send()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/storage/stores.go:185 +0x133
  github.com/cockroachdb/cockroach/pkg/server.(*Node).batchInternal.func1()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/server/node.go:1009 +0x1eb
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunTaskWithErr()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:303 +0xfa
  github.com/cockroachdb/cockroach/pkg/server.(*Node).batchInternal()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/server/node.go:996 +0x362
  github.com/cockroachdb/cockroach/pkg/server.(*Node).Batch()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/server/node.go:1037 +0xba
  github.com/cockroachdb/cockroach/pkg/rpc.internalClientAdapter.Batch()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/rpc/context.go:375 +0x63
  github.com/cockroachdb/cockroach/pkg/rpc.(*internalClientAdapter).Batch()
      <autogenerated>:1 +0xc0
  github.com/cockroachdb/cockroach/pkg/kv.(*grpcTransport).sendBatch()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/transport.go:199 +0x18d
  github.com/cockroachdb/cockroach/pkg/kv.(*grpcTransport).SendNext()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/transport.go:169 +0x1a9
  github.com/cockroachdb/cockroach/pkg/kv.raceTransport.SendNext()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/transport_race.go:70 +0x154
  github.com/cockroachdb/cockroach/pkg/kv.(*raceTransport).SendNext()
      <autogenerated>:1 +0xca
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).sendToReplicas()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:1306 +0x3a5
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).sendRPC()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:387 +0x2cb
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).sendSingleRange()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:457 +0x1ef
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).sendPartialBatch()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:1083 +0x3da
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).divideAndSendBatchToRanges()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:759 +0x17d4
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).Send()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:671 +0x683
  github.com/cockroachdb/cockroach/pkg/internal/client.(*CrossRangeTxnWrapperSender).Send()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:213 +0xe3
  github.com/cockroachdb/cockroach/pkg/internal/client.lookupRangeFwdScan()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/range_lookup.go:319 +0x51e
  github.com/cockroachdb/cockroach/pkg/internal/client.RangeLookup()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/range_lookup.go:202 +0xa43
  github.com/cockroachdb/cockroach/pkg/server.(*TestServer).LookupRange()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/server/testserver.go:695 +0xf7
  github.com/cockroachdb/cockroach/pkg/sql_test.TestSplitAt()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/sql/split_test.go:144 +0xb2e
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Previous write at 0x00c421ade54c by goroutine 196:
  github.com/cockroachdb/cockroach/pkg/roachpb.(*Transaction).Update()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/roachpb/data.go:1008 +0xa49
  github.com/cockroachdb/cockroach/pkg/roachpb.(*BatchResponse_Header).combine()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/roachpb/api.go:389 +0x244
  github.com/cockroachdb/cockroach/pkg/roachpb.(*BatchResponse).Combine()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/roachpb/batch.go:339 +0x9b
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).divideAndSendBatchToRanges.func1()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:809 +0x216
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).divideAndSendBatchToRanges()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:974 +0x91e
  github.com/cockroachdb/cockroach/pkg/kv.(*DistSender).Send()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/dist_sender.go:671 +0x683
  github.com/cockroachdb/cockroach/pkg/kv.(*txnLockGatekeeper).SendLocked()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/txn_coord_sender.go:234 +0x144
  github.com/cockroachdb/cockroach/pkg/kv.(*txnMetrics).SendLocked()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/txn_interceptor_metrics.go:57 +0x197
  github.com/cockroachdb/cockroach/pkg/kv.(*txnSpanRefresher).sendLockedWithRefreshAttempts()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/txn_interceptor_span_refresher.go:159 +0xbd
  github.com/cockroachdb/cockroach/pkg/kv.(*txnSpanRefresher).SendLocked()
      /home/tschottdorf/go/src/github.com/cockroachdb/cockroach/pkg/kv/txn_interceptor_span_refresher.go:105 +0x151
  github.com/cockroachdb/cockroach/pkg/kv.(*txnPipeliner).SendLocked()

@tbg
Copy link
Member Author

tbg commented Sep 25, 2018

This is interesting. I think the test tickles that race because it doesn't turn off range merges (which it supposedly wants to do to work better). The AdminMerge txn somehow leaks its txn into the contention queue where it is read by another waiting txn while the DistSender mutates it (in a returning request from the merge txn).

@tbg tbg requested review from a team, nvanbenschoten and andreimatei September 25, 2018 12:46
@tbg
storage: fix data race in contentionQueue
85d5ab6 
When a pusher would write a new intent, it would previously sent its own
*TxnMeta to the next pusher. However, it would also possibly mutate it
higher up the stack (for example combining multiple responses in
DistSender). Make a copy instead.

Release note: None
@tbg
Copy link
Member Author

tbg commented Sep 25, 2018

Ok, fixed the data race. And the test is well aware of range merges and in fact turns them on and off on purpose. So this should be good to go now.

nvanbenschoten
nvanbenschoten approved these changes Sep 25, 2018
View reviewed changes
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@tbg
Copy link
Member Author

tbg commented Sep 25, 2018

bors r=nvanbenschoten

craig bot pushed a commit that referenced this pull request Sep 25, 2018
@tbg
Merge #30623
15e4c8d 
30623: sql: unskip TestSplitAt r=nvanbenschoten a=tschottdorf

The failure was fixed in #29324.

Closes #29169.

Release note: None

Co-authored-by: Tobias Schottdorf <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 25, 2018

Build succeeded

@craig craig bot merged commit 85d5ab6 into cockroachdb:master Sep 25, 2018
@tbg tbg mentioned this pull request Sep 27, 2018
Merged
@tbg tbg deleted the fix/TestSplitAt branch November 23, 2018 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvanbenschoten nvanbenschoten nvanbenschoten approved these changes

@andreimatei andreimatei Awaiting requested review from andreimatei

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

Successfully merging this pull request may close these issues.

sql: TestSplitAt failed under stress
3 participants
@tbg @cockroach-teamcity @nvanbenschoten