{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/12352","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/12352/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/12352/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/12352/events","html_url":"https://github.com/cockroachdb/cockroach/pull/12352","id":195640683,"number":12352,"title":"storage: avoid node liveness increment and heartbeat pile-ons","user":{"login":"spencerkimball","id":6147149,"avatar_url":"https://avatars.githubusercontent.com/u/6147149?v=3","gravatar_id":"","url":"https://api.github.com/users/spencerkimball","html_url":"https://github.com/spencerkimball","followers_url":"https://api.github.com/users/spencerkimball/followers","following_url":"https://api.github.com/users/spencerkimball/following{/other_user}","gists_url":"https://api.github.com/users/spencerkimball/gists{/gist_id}","starred_url":"https://api.github.com/users/spencerkimball/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spencerkimball/subscriptions","organizations_url":"https://api.github.com/users/spencerkimball/orgs","repos_url":"https://api.github.com/users/spencerkimball/repos","events_url":"https://api.github.com/users/spencerkimball/events{/privacy}","received_events_url":"https://api.github.com/users/spencerkimball/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"assignees":[{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false}],"milestone":null,"comments":12,"created_at":"2016-12-14T20:43:28Z","updated_at":"2016-12-19T20:02:18Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/12352","html_url":"https://github.com/cockroachdb/cockroach/pull/12352","diff_url":"https://github.com/cockroachdb/cockroach/pull/12352.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/12352.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/12352)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/268063450","html_url":"https://github.com/cockroachdb/cockroach/pull/12352#issuecomment-268063450","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/12352","id":268063450,"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-12-19T20:02:18Z","updated_at":"2016-12-19T20:02:18Z","body":"\n\n\n\nReviewed 1 of 1 files at r4.\nReview status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks pending.\n\n---\n\n*[pkg/storage/node_liveness.go, line 214 at r4](https://reviewable.io:443/reviews/cockroachdb/cockroach/12352#-KZNee998rQdPQTV0631:-KZNee9ArhCRq4UngWQo:b-qpx8ja) ([raw file](https://github.com/cockroachdb/cockroach/blob/d29156a25099b1bf0c870235c6861ffc16042377/pkg/storage/node_liveness.go#L214)):*\n> ```Go\n> // successful, stores the updated liveness record in the nodes map.\n> func (nl *NodeLiveness) Heartbeat(ctx context.Context, liveness *Liveness) error {\n> \t// Allow only one heartbeat at a time.\n> ```\n\nmight make more sense to put this on the `make(chan struct{}, 1)` call\n\n---\n\n*[pkg/storage/node_liveness.go, line 242 at r4](https://reviewable.io:443/reviews/cockroachdb/cockroach/12352#-KZNf6ct0xPdjR-ab70g:-KZNf6cuQtUridDPHuXz:bs7d32r) ([raw file](https://github.com/cockroachdb/cockroach/blob/17b99047a726d5c3c4d8bf595812a27abdaf16ff/pkg/storage/node_liveness.go#L242)):*\n> ```Go\n> \tnl.mu.Unlock()\n> \tif err != nil {\n> \t\tnl.metrics.HeartbeatFailures.Inc(1)\n> ```\n\ncan you revert this bit if the answer to my above question is \"no\"?\n\n---\n\n*[pkg/storage/node_liveness.go, line 256 at r4](https://reviewable.io:443/reviews/cockroachdb/cockroach/12352#-KZNf8FTiXshQCLAQyih:-KZNf8FTiXshQCLAQyii:box14aq) ([raw file](https://github.com/cockroachdb/cockroach/blob/d29156a25099b1bf0c870235c6861ffc16042377/pkg/storage/node_liveness.go#L256)):*\n> ```Go\n> \t\t\treturn nil\n> \t\t}\n> \t\tnl.metrics.HeartbeatFailures.Inc(1)\n> ```\n\nyou want this incremented even in the case of `errNodeAlreadyAlive`?\n\n---\n\n*[pkg/storage/node_liveness.go, line 265 at r4](https://reviewable.io:443/reviews/cockroachdb/cockroach/12352#-KZNfIf84kqu_ZsFuBHA:-KZNfIf84kqu_ZsFuBHB:bzafaz2) ([raw file](https://github.com/cockroachdb/cockroach/blob/d29156a25099b1bf0c870235c6861ffc16042377/pkg/storage/node_liveness.go#L265)):*\n> ```Go\n> \tnl.mu.Unlock()\n> \tnl.metrics.HeartbeatSuccesses.Inc(1)\n> \treturn nil\n> ```\n\nthis should return err - bad conflict resolution?\n\n---\n\n*[pkg/storage/node_liveness.go, line 324 at r4](https://reviewable.io:443/reviews/cockroachdb/cockroach/12352#-KZNemfMqping_5vB7OY:-KZNemfMqping_5vB7OZ:b-7ropln) ([raw file](https://github.com/cockroachdb/cockroach/blob/d29156a25099b1bf0c870235c6861ffc16042377/pkg/storage/node_liveness.go#L324)):*\n> ```Go\n> // gathered through gossip, an error is returned.\n> func (nl *NodeLiveness) IncrementEpoch(ctx context.Context, liveness *Liveness) error {\n> \t// Allow only one increment at a time.\n> ```\n\nditto\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/12352)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tamird"}}