{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16399","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16399/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16399/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16399/events","html_url":"https://github.com/cockroachdb/cockroach/pull/16399","id":234361901,"number":16399,"title":"storage: deflake TestRaftRemoveRace","user":{"login":"irfansharif","id":10536690,"avatar_url":"https://avatars0.githubusercontent.com/u/10536690?v=3","gravatar_id":"","url":"https://api.github.com/users/irfansharif","html_url":"https://github.com/irfansharif","followers_url":"https://api.github.com/users/irfansharif/followers","following_url":"https://api.github.com/users/irfansharif/following{/other_user}","gists_url":"https://api.github.com/users/irfansharif/gists{/gist_id}","starred_url":"https://api.github.com/users/irfansharif/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/irfansharif/subscriptions","organizations_url":"https://api.github.com/users/irfansharif/orgs","repos_url":"https://api.github.com/users/irfansharif/repos","events_url":"https://api.github.com/users/irfansharif/events{/privacy}","received_events_url":"https://api.github.com/users/irfansharif/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2017-06-07T22:32:17Z","updated_at":"2017-06-08T16:56:43Z","closed_at":"2017-06-08T14:51:53Z","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/16399","html_url":"https://github.com/cockroachdb/cockroach/pull/16399","diff_url":"https://github.com/cockroachdb/cockroach/pull/16399.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/16399.patch"},"body":"Fixes #16376.\r\n\r\nUnder stress TestRaftRemoveRace failed with:\r\n\r\n    --- FAIL: TestRaftRemoveRace (1.45s)\r\n      client_test.go:1031: change replicas of r1 failed: quota pool no longer in use\r\n\r\nConsider the following:\r\n- 'add replica' commands get queued up on the replicate queue\r\n- leader replica steps down as leader due to timeouts thus closing the\r\n  quota pool\r\n- commands get out of the queue, cannot acquire quota because the quota\r\n  pool is closed and fail with an error indicating so\r\n\r\nTestRaftRemoveRace fails as it expects all replica additions to go\r\nthrough without failure.\r\n\r\nTo reproduce this the following minimal test (run under stress) is\r\nsufficient where we lower RaftTickInterval and RaftElectionTimeoutTicks\r\nto make it more likely that leadership changes take place.\r\n\r\n    func TestReplicateQueueQuota(t *testing.T) {\r\n      defer leaktest.AfterTest(t)()\r\n        sc := storage.TestStoreConfig(nil)\r\n        sc.RaftElectionTimeoutTicks = 2             // Default: 15\r\n        sc.RaftTickInterval = 10 * time.Millisecond // Default: 200ms\r\n        mtc := &multiTestContext{storeConfig: &sc}\r\n        defer mtc.Stop()\r\n        mtc.Start(t, 3)\r\n\r\n        const rangeID = roachpb.RangeID(1)\r\n        mtc.replicateRange(rangeID, 1, 2)\r\n\r\n        for i := 0; i < 10; i++ {\r\n            mtc.unreplicateRange(rangeID, 2)\r\n            mtc.replicateRange(rangeID, 2)\r\n        }\r\n    }\r\n\r\nThe earlier version of TestRaftRemoveRace was written to reproduce the\r\nfailure seen in #9037, this was agnostic to raft leadership changes."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/307163932","html_url":"https://github.com/cockroachdb/cockroach/pull/16399#issuecomment-307163932","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16399","id":307163932,"user":{"login":"bdarnell","id":160562,"avatar_url":"https://avatars2.githubusercontent.com/u/160562?v=3","gravatar_id":"","url":"https://api.github.com/users/bdarnell","html_url":"https://github.com/bdarnell","followers_url":"https://api.github.com/users/bdarnell/followers","following_url":"https://api.github.com/users/bdarnell/following{/other_user}","gists_url":"https://api.github.com/users/bdarnell/gists{/gist_id}","starred_url":"https://api.github.com/users/bdarnell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bdarnell/subscriptions","organizations_url":"https://api.github.com/users/bdarnell/orgs","repos_url":"https://api.github.com/users/bdarnell/repos","events_url":"https://api.github.com/users/bdarnell/events{/privacy}","received_events_url":"https://api.github.com/users/bdarnell/received_events","type":"User","site_admin":false},"created_at":"2017-06-08T16:56:43Z","updated_at":"2017-06-08T16:56:43Z","body":"We require that raft commands are proposed on the *lease holder*, not the *raft leader* (the lease holder will forward the proposal to the leader when this occurs). That's what was happening in the test (the ChangeReplicas transaction was always sent to the lease holder). I think this test was uncovering an actual bug in the quota pool. \n\n---\n\nReview status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.\n\n---\n\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/16399#-:-Km7c9s6_WPfZy8etu1a:b-xymmg)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"bdarnell"}}