{"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":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":23,"created_at":"2016-09-14T21:21:59Z","updated_at":"2016-10-03T21:10:19Z","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/251228098","html_url":"https://github.com/cockroachdb/cockroach/pull/9380#issuecomment-251228098","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9380","id":251228098,"user":{"login":"tamird","id":1535036,"avatar_url":"https://avatars.githubusercontent.com/u/1535036?v=3","gravatar_id":"","url":"https://api.github.com/users/tamird","html_url":"https://github.com/tamird","followers_url":"https://api.github.com/users/tamird/followers","following_url":"https://api.github.com/users/tamird/following{/other_user}","gists_url":"https://api.github.com/users/tamird/gists{/gist_id}","starred_url":"https://api.github.com/users/tamird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tamird/subscriptions","organizations_url":"https://api.github.com/users/tamird/orgs","repos_url":"https://api.github.com/users/tamird/repos","events_url":"https://api.github.com/users/tamird/events{/privacy}","received_events_url":"https://api.github.com/users/tamird/received_events","type":"User","site_admin":false},"created_at":"2016-10-03T21:10:19Z","updated_at":"2016-10-03T21:10:19Z","body":"\n\n\n\nReviewed 1 of 9 files at r8, 1 of 1 files at r9.\nReview status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks pending.\n\n---\n\n*[storage/replica.go, line 2332 at r6](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KSlI3NZqTTww-Cl2vfo:-KTBNOTQa-CgAhpYdJ83:boywe2u) ([raw file](https://github.com/cockroachdb/cockroach/blob/f4cf6138cf0eb3b64bbb5ab97456591d6f84b589/storage/replica.go#L2332)):*\n<details><summary><i>Previously, arjunravinarayan (Arjun Narayan) wrote…</i></summary>\n> The map gets cleared a lot, I think it's faster this way. Holding the lock for this little extra bit here is better.\n</details>\nI think you're trading garbage for convenience, but I don't feel strongly enough to insist.\n\n---\n\n*[storage/store.go, line 2942 at r9](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380#-KTBNARgfpJylmXC2wGU:-KTBNARhJ6Kg2EfNGNg9:b-z9kq1e) ([raw file](https://github.com/cockroachdb/cockroach/blob/2f142d1de45e0f2c8daf70a5c6925ee08bdeca70/storage/store.go#L2942)):*\n> ```Go\n> \n> // sendQueuedHeartbeatsToNode requires that the s.coalescedMu lock is held. It\n> // returns the number of heartbeats that were sent.\n> ```\n\nwait, is this true? it seems that it returns the number of heartbeats that were passed, regardless of them being sent or not.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/9380)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tamird"}}