{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9380","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9380/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9380/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9380/events","html_url":"https://github.com/cockroachdb/cockroach/pull/9380","id":177024787,"number":9380,"title":"storage: coalesce heartbeats","user":{"login":"arjunravinarayan","id":1565833,"avatar_url":"https://avatars.githubusercontent.com/u/1565833?v=3","gravatar_id":"","url":"https://api.github.com/users/arjunravinarayan","html_url":"https://github.com/arjunravinarayan","followers_url":"https://api.github.com/users/arjunravinarayan/followers","following_url":"https://api.github.com/users/arjunravinarayan/following{/other_user}","gists_url":"https://api.github.com/users/arjunravinarayan/gists{/gist_id}","starred_url":"https://api.github.com/users/arjunravinarayan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arjunravinarayan/subscriptions","organizations_url":"https://api.github.com/users/arjunravinarayan/orgs","repos_url":"https://api.github.com/users/arjunravinarayan/repos","events_url":"https://api.github.com/users/arjunravinarayan/events{/privacy}","received_events_url":"https://api.github.com/users/arjunravinarayan/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/stability","name":"stability","color":"b60205"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":33,"created_at":"2016-09-14T21:21:59Z","updated_at":"2016-10-04T21:14:01Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/9380","html_url":"https://github.com/cockroachdb/cockroach/pull/9380","diff_url":"https://github.com/cockroachdb/cockroach/pull/9380.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/9380.patch"},"body":"Coalesce heartbeats and heartbeat responses bound for the same store\r\ninto a single proto. Introduce a new environment flag (default: true\r\nto enable this in integration tests, will be changed to false before\r\nmerging) COCKROACH_ENABLE_COALESCED_HEARTBEATS to turn this feature\r\non. Added metrics and a graph in the admin UI to track the number of\r\nqueued heartbeats waiting to be coalesced.\r\n\r\nUnlike the earlier proposal for coalesced heartbeats which would contain zero\r\nadditional information, this version sends large heartbeat packets that wrap up\r\nall the individual heartbeat messages.\r\n\r\nI would appreciate feedback on the following points:\r\n1. Should I use a slice locked by a mutex for the heartbeat message queue, \r\nas opposed to the current channel based implementation?\r\n2. Am I including sufficient information in the reconstructed heartbeat message?\r\n3. Are there any other metrics that we would like to collect and graph to make this clearer?\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/9380)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/251515612","html_url":"https://github.com/cockroachdb/cockroach/pull/9380#issuecomment-251515612","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9380","id":251515612,"user":{"login":"arjunravinarayan","id":1565833,"avatar_url":"https://avatars.githubusercontent.com/u/1565833?v=3","gravatar_id":"","url":"https://api.github.com/users/arjunravinarayan","html_url":"https://github.com/arjunravinarayan","followers_url":"https://api.github.com/users/arjunravinarayan/followers","following_url":"https://api.github.com/users/arjunravinarayan/following{/other_user}","gists_url":"https://api.github.com/users/arjunravinarayan/gists{/gist_id}","starred_url":"https://api.github.com/users/arjunravinarayan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arjunravinarayan/subscriptions","organizations_url":"https://api.github.com/users/arjunravinarayan/orgs","repos_url":"https://api.github.com/users/arjunravinarayan/repos","events_url":"https://api.github.com/users/arjunravinarayan/events{/privacy}","received_events_url":"https://api.github.com/users/arjunravinarayan/received_events","type":"User","site_admin":false},"created_at":"2016-10-04T21:14:01Z","updated_at":"2016-10-04T21:14:01Z","body":"Re testing: the existing `TestRaftHeartbeats` was dormant since coalesced heartbeats was removed. It tests if coalesced heartbeats destabilizes raft leadership. I think that's sufficient, but let me know if there's something else you would like to see.\nRe `TestStopServer`: saw this again, seeing it within 1k runs under stress, so taking a look at this now. Should be the last outstanding task.\n\n---\n\nReview status: 3 of 10 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.\n\n---\n\n*[storage/client_raft_test.go, line 1695 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTExC9hNvrc1AAmP_Up:-KTFj12uZLqBjB0z4gX4:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/client_raft_test.go#L1695)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Punctuation.\n</details>\nDone.\n\n---\n\n*[storage/metrics.go, line 480 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTExP4zpqV0f_yaHwRF:-KTFunmvtxj19ZAc81Qg:bmoo3xw) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/metrics.go#L480)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Is this useful? It would fluctuate wildly when you actually read it. I think it should be set every time heartbeats *are* being coalesced (i.e. every time you grab all of the ones that have queued up). Then this would measure approximately how many heartbeats are actually coalesced in each cycle.\n> \n> Maybe that's what's happening, in which case please update the description above to that effect.\n</details>\nThat is what is happening, for this exact reason, but you are right: the comment is certainly needed! Updated.\n\n---\n\n*[storage/raft.proto, line 26 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTExx8ljghR34NsAlq7:-KTFjVpiRVxH6g-AFnzR:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/raft.proto#L26)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Mention the origin of this proto.\n</details>\nDone.\n\n---\n\n*[storage/raft.proto, line 63 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF-EBCNOPUISv9QCCO:-KTFjytONQ76fibtnRhn:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/raft.proto#L63)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> This comment is generally stale. What's `coalesced_heartbeat`?\n</details>\nDone.\n\n---\n\n*[storage/raft.proto, line 65 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF-KWGaBgn8yJLzU_K:-KTFk3yLpTXXIm0xN7iI:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/raft.proto#L65)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> The discard part should be in the proto's main comment.\n</details>\nDone.\n\n---\n\n*[storage/raft.proto, line 66 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTEy4GvXmMA04RoBBjY:-KTFk4KUrqQONcyla3hS:b-qiexsz) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/raft.proto#L66)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> \"should\" is FUD. \"Must be\" (and assert on it).\n</details>\nDone. Added a panic if a coalesced heartbeat is received with a nonzero range ID.\n\n---\n\n*[storage/replica.go, line 2244 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF0k3askYBki0GGqI5:-KTFphvOKXfrJtRapQXI:blmkezy) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/replica.go#L2244)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> In the nonexistent `else` branch, aren't you done with this message? Why aren't you returning? ISTM that when you can't coalesce the heartbeat, *then* you would continue sending a \"real\" one.\n</details>\nChanged behavior here, since we are checking the flag in `maybeCoalesceHeartbeat`. The behavior now is to send a non-coalesced quiesce message if we cannot send a coalesced quiesce message.\n\n---\n\n*[storage/replica.go, line 2352 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF0eyaUwzYjIJh7K3Z:-KTFpwh-ItkIYGJSYW5D:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/replica.go#L2352)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Comment here (mostly about the returned value).\n</details>\nDone.\n\n---\n\n*[storage/replica.go, line 2355 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF0Q2N-JSbie8yPkln:-KTFq_YzhwY7OAVeizOK:bydxth8) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/replica.go#L2355)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> This seems to already be checked at the callsite above?\n</details>\nAll calls of `maybeCoalesceHeartbeat` now do not check, the check is only done here.\n\n---\n\n*[storage/replica.go, line 2355 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF1AiDOgTLEz9RZHCY:-KTFqn5Jo27Ic4UCIxUC:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/replica.go#L2355)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Besides, you can save a lot of indentation by making this an early return:\n> \n> ```go\n> if !r.store.ctx.enableCoalescedHeartbeats {\n>   return false\n> }\n> ```\n</details>\nDone.\n\n---\n\n*[storage/replica.go, line 2465 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF1Qo6frR4WfUoSyB2:-KTFwRTF50AZo-HyW_lS:bf0man1) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/replica.go#L2465)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> It would be odd if a quiescing heartbeat made it here, seeing that you're passing `quiesce=false`. Can't you always reconstruct that last parameter from the message?\n</details>\n`maybeCoalesceHeartbeat` has two callers, this one (with `quiesce=false`), and from `maybeQuiesceLocked` (with `quiesce=true`). Unfortunately, `msg` doesn't have quiesce information, that is a flag in the wrapping `RaftMessageRequest`, which we have not yet constructed at this point. \n\n---\n\n*[storage/store.go, line 532 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTExe-nKPZsjenDj34M:-KTG2CpqtohxlZ8TO61_:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L532)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary>\n> Do you ever disable this in tests? If not, it would be simpler to make this a global like `enableQuiescence`. \n</details>\nDone.\n\n---\n\n*[storage/store.go, line 2364 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTExFO8QDvBxFJMs9Lt:-KTFiDtaw8bhpFGJRY8D:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L2364)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary>\n> Spurious blank line.\n</details>\nDone.\n\n---\n\n*[storage/store.go, line 2386 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTExK2rBsR7wFx3pFCh:-KTFiODncQFV7jxVYpWw:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L2386)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary>\n> For symmetry, make this expression the same as the one in `HandleRaftRequest`.\n</details>\nDone.\n\n---\n\n*[storage/store.go, line 2387 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF23O5btAZ-mJp7Mqv:-KTFiUD5W9grG0Pbedq-:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L2387)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Print what was actually supplied.\n</details>\nDone.\n\n---\n\n*[storage/store.go, line 2389 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF2A-BUNo277YtpvEQ:-KTG3IJjHe2w43mLBclX:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L2389)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Update the various metrics that deal with Raft traffic so that they're clear on where coalesced heartbeats and their responses are counted vs their individual heartbeats. I think what *should* be the case is that your new gauge approximately gives the number of heartbeats and responses (separately) per cycle, and that otherwise a heartbeat message bundle counts as one.\n</details>\nDone.\n\n---\n\n*[storage/store.go, line 2976 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF3PgOxaeomYgfsw1D:-KTGDrB6kEyMxEiLYzR6:bhzy1dj) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L2976)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> This is a lock order violation. We may acquire `s.mu` from `*Replica` and as a consequence it's bad to hold `s.mu` while doing anything with a *Replica that needs (really any of its) locks. I think the only reason the tests aren't going to catch this is a) because we rarely run with `TAGS=deadlock` and b) because we don't have enough test coverage of this.\n> \n> Just copy out the replicas from under the lock for now.\n</details>\nI *think* I've done what you meant.\n\n---\n\n*[storage/store.go, line 2993 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF43dAiIZfbcHQCK4Q:-KTFieZWjhvXCKbo7vJz:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/storage/store.go#L2993)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> This isn't called anywhere else, so just inline it here.\n</details>\nDone.\n\n---\n\n*[ui/app/containers/nodeGraphs.tsx, line 258 at r11](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTF4z-hW6qYxpDSl6PU:-KTG3Y4Uwi5VPWZ5Buiv:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/b63edc5e668dbb2781b89795d8bb40d5c4856b0f/ui/app/containers/nodeGraphs.tsx#L258)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> Update this description (see the comment on the metric).\n</details>\nDone.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-:-KTFigiUu1E3lx0_aRSU:b-hi4xkc)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"arjunravinarayan"}}