{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5257","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5257/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5257/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5257/events","html_url":"https://github.com/cockroachdb/cockroach/pull/5257","id":140878998,"number":5257,"title":"Added high-content benchmarks and fixed starvation bugs","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":"tschottdorf","id":5076964,"avatar_url":"https://avatars.githubusercontent.com/u/5076964?v=3","gravatar_id":"","url":"https://api.github.com/users/tschottdorf","html_url":"https://github.com/tschottdorf","followers_url":"https://api.github.com/users/tschottdorf/followers","following_url":"https://api.github.com/users/tschottdorf/following{/other_user}","gists_url":"https://api.github.com/users/tschottdorf/gists{/gist_id}","starred_url":"https://api.github.com/users/tschottdorf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tschottdorf/subscriptions","organizations_url":"https://api.github.com/users/tschottdorf/orgs","repos_url":"https://api.github.com/users/tschottdorf/repos","events_url":"https://api.github.com/users/tschottdorf/events{/privacy}","received_events_url":"https://api.github.com/users/tschottdorf/received_events","type":"User","site_admin":false},"milestone":null,"comments":4,"created_at":"2016-03-15T05:35:18Z","updated_at":"2016-03-15T15:58:47Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/5257","html_url":"https://github.com/cockroachdb/cockroach/pull/5257","diff_url":"https://github.com/cockroachdb/cockroach/pull/5257.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/5257.patch"},"body":"The bank benchmark was never doing anything before now as there\r\nwas no money in any of the accounts, so nothing would transfer.\r\n\r\nAdded high-contention workloads to the benchmarks and in the\r\nprocess discovered starvation scenarios and an outright bug.\r\n\r\nThe starvation scenario was encountered when a batch containing\r\nwrites and an end transaction failed due to a serialized txn\r\nhaving `Timestamp` != `OrigTimestamp`. In that event, we discard\r\nthe entire batch engine, meaning the intents for the writes are\r\ndiscarded with everything else.\r\n\r\nThis change commits the results of a batch which fails with\r\nEndTransaction.\r\n\r\nThere was also a bug when restarting a transaction. The replay\r\nof the `BeginTransaction` statement was incorrectly using the\r\nvalue of the transaction record instead of the transaction\r\npassed in with the batch request. In the event that the txn\r\nwas pushed or aborted during the restart, this would cause\r\nvarious problems when commit time comes.\r\n\r\nThe 2, 4, and 8 account benchmarks are pretty slow, but\r\noptimizing those is the next step.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/5257)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/196895500","html_url":"https://github.com/cockroachdb/cockroach/pull/5257#issuecomment-196895500","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5257","id":196895500,"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},"created_at":"2016-03-15T15:58:46Z","updated_at":"2016-03-15T15:58:46Z","body":"\n\n\n\nReview status: all files reviewed at latest revision, 8 unresolved discussions.\n\n---\n\n*[sql/bank_test.go, line 65 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5257#-KCuLv5mpbWxHy2-9UQu:-KCur904EDWkz80nIH35:-1391749973) ([raw file](https://github.com/cockroachdb/cockroach/blob/0d266955c81bb1cda380b337807b77b241145502/sql/bank_test.go#L65)):*\nIf the parallelism is set to 1, for example, nothing makes sense in terms of the benchmark. You'd get zero contention. It's crucial we have a set level of parallelism here. Should I just assume it will be run with `-cpu 4`?\n\n---\n\n*[sql/bank_test.go, line 69 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5257#-KCuM8owRzagY1ui6IS8:-KCusQxDqGMp1HUJGWl7:-1823842311) ([raw file](https://github.com/cockroachdb/cockroach/blob/0d266955c81bb1cda380b337807b77b241145502/sql/bank_test.go#L69)):*\nThe previous code was biased towards selecting `*numAccounts - 1`, which is not correct if trying to select a uniformly random account. It's not possible to select `to == from` with this code. The `for` loop condition prevents it.\n\n---\n\n*[sql/bank_test.go, line 89 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5257#-KCuZ9K2EYzlfPo9jL00:-KCusxgfZh7uZbKDgGpx:5173665) ([raw file](https://github.com/cockroachdb/cockroach/blob/0d266955c81bb1cda380b337807b77b241145502/sql/bank_test.go#L89)):*\nOK\n\n---\n\n*[storage/replica.go, line 1510 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5257#-KCurDyqfpOkVB9UUuBI:-KCv-1bE-krcxYLGMpJl:-502253829) ([raw file](https://github.com/cockroachdb/cockroach/blob/0d266955c81bb1cda380b337807b77b241145502/storage/replica.go#L1510)):*\nAdded\n\n---\n\n*[storage/replica_command.go, line 325 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5257#-KCuYDe0dvLTrLUY8JJA:-KCuvhP8_Nfrv_m2x033:959752069) ([raw file](https://github.com/cockroachdb/cockroach/blob/0d266955c81bb1cda380b337807b77b241145502/storage/replica_command.go#L325)):*\nFixed comment and updated request txn from extant txn record and am now rewriting it.\n\n---\n\n\n*Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/cockroachdb/cockroach/5257)*\n<!-- Sent from Reviewable.io -->\n"}}