{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10507","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10507/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10507/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10507/events","html_url":"https://github.com/cockroachdb/cockroach/pull/10507","id":187754304,"number":10507,"title":"storage: Re-apply the rule solver.","user":{"login":"BramGruneir","id":1614265,"avatar_url":"https://avatars.githubusercontent.com/u/1614265?v=3","gravatar_id":"","url":"https://api.github.com/users/BramGruneir","html_url":"https://github.com/BramGruneir","followers_url":"https://api.github.com/users/BramGruneir/followers","following_url":"https://api.github.com/users/BramGruneir/following{/other_user}","gists_url":"https://api.github.com/users/BramGruneir/gists{/gist_id}","starred_url":"https://api.github.com/users/BramGruneir/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/BramGruneir/subscriptions","organizations_url":"https://api.github.com/users/BramGruneir/orgs","repos_url":"https://api.github.com/users/BramGruneir/repos","events_url":"https://api.github.com/users/BramGruneir/events{/privacy}","received_events_url":"https://api.github.com/users/BramGruneir/received_events","type":"User","site_admin":false},"labels":[{"id":317051482,"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/docs-todo","name":"docs-todo","color":"5319e7","default":false},{"id":371606733,"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/stability","name":"stability","color":"b60205","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2016-11-07T16:14:15Z","updated_at":"2016-11-09T05:38:24Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/10507","html_url":"https://github.com/cockroachdb/cockroach/pull/10507","diff_url":"https://github.com/cockroachdb/cockroach/pull/10507.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/10507.patch"},"body":"Instead of applying https://github.com/cockroachdb/cockroach/pull/10252/commits/1ef40f323b94005dd84378669da8296c9fec8e7c or #10252, this finishes the\r\nreapplication of the rule solver. However, this also puts the rule solver under\r\nthe environment flag COCKROACH_ENABLE_RULE_SOLVER for ease of testing and defaults\r\nto not enabled.\r\n\r\nThe follow up to this commit is #10275 and a lot of testing to ensure that the\r\nrule solver does indeed perform as expected.\r\n\r\nCloses #9336\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/10507)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/259337659","html_url":"https://github.com/cockroachdb/cockroach/pull/10507#issuecomment-259337659","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10507","id":259337659,"user":{"login":"BramGruneir","id":1614265,"avatar_url":"https://avatars.githubusercontent.com/u/1614265?v=3","gravatar_id":"","url":"https://api.github.com/users/BramGruneir","html_url":"https://github.com/BramGruneir","followers_url":"https://api.github.com/users/BramGruneir/followers","following_url":"https://api.github.com/users/BramGruneir/following{/other_user}","gists_url":"https://api.github.com/users/BramGruneir/gists{/gist_id}","starred_url":"https://api.github.com/users/BramGruneir/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/BramGruneir/subscriptions","organizations_url":"https://api.github.com/users/BramGruneir/orgs","repos_url":"https://api.github.com/users/BramGruneir/repos","events_url":"https://api.github.com/users/BramGruneir/events{/privacy}","received_events_url":"https://api.github.com/users/BramGruneir/received_events","type":"User","site_admin":false},"created_at":"2016-11-09T05:38:24Z","updated_at":"2016-11-09T05:38:24Z","body":"Addressed all the comments and added more info the commit message.\n\n---\n\nReview status: 1 of 8 files reviewed at latest revision, 46 unresolved discussions, some commit checks pending.\n\n---\n\n*[pkg/storage/allocator.go, line 145 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzau3e69H3RMgExigB:-KVzu7TujWClChqA1fGb:bgzrgw7) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L145)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> can you add TODOs to those tests, or is it essential that they not construct a gossip or store pool?\n</details>\nThey call store.New, but don't need to do anything with the allocator or any networking and adding a gossip and store pool will just bloat the tests needlessly.  I'm going to leave them as is.\n\n---\n\n*[pkg/storage/allocator.go, line 296 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzejUcrOnsoHD9LVAP:-KVzukmAuKVwDghlN7x7:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L296)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit: worstScore := math.Inf(0)\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 305 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzeE-7tpttbwDHw3NC:-KVzvBIGx0RPc0sxsQ0W:b-bv49eg) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L305)):*\nThe bool result is irrelevant here, what we're looking for is the worst of the candidate.  \n\nAs mentioned in the last review of this code:\n> That's a bool whether or not the that store is a potential candidate. If it's not a candidate, the score will be zero and thus will be the worst. Just makes the logic simpler.\n\nBut since this is the second time you've wondered about this code, I'm just going to explicitly check the bool to make it clearer.\n\n---\n\n*[pkg/storage/allocator.go, line 320 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzexCcDLGbSfOT3oU0:-KW--if8ZVjC0wiPuRBo:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L320)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> `if math.IsInf(worstScore, 0) {`?\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 324 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzf16_O-wyxh8h5TCf:-KW--nv7pvehr6gSfUzj:b-sxrzhp) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L324)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> errors.New, but also this error should not include the method name IMO; the caller should/would attach that using errors.Wrapf. This is just my intuition of the convention, so I would accept a counter-argument.\n</details>\nDone.  Fixed both instances (with and without rule solver)\n\n---\n\n*[pkg/storage/allocator.go, line 400 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzfHxrOFTCj0XGtMue:-KW-0Bf5Zq-_aZuGYJBj:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L400)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> not a helpful comment. perhaps explain why this done instead.\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 406 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzfNrnVipOS7n6WgQ6:-KW-0Zx8NlemG-ZJyhsu:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L406)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> not a helpful comment. perhaps explain why this done instead.\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 431 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzfgT1B61ZLu_GAid2:-KW-10-MXDTICPtYtXI5:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L431)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this sentence mixes past and present tenses. s/was/is/\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 432 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzfWqH5_5kqQ69MaMZ:-KW-10iVIc4ZYoIYm56d:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L432)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/Constrains. S/constrains, s/\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 440 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzflvdcY2atKinBnyN:-KW-1Er_T2n7t043eyeF:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L440)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> these parentheses are superfluous\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator.go, line 507 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzfuaZnliiPpoxX8Kz:-KW6_7qB-pr9dh92HnUP:b-fui5qn) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L507)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> which tests?\n</details>\nThis is an old and odd comment.  I'm not entirely sure what it's referencing.  Obviously we have rebalancing tests. \n\n---\n\n*[pkg/storage/allocator.go, line 510 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzg6bNMBLXEJin5EcK:-KW-2QCSiZs6k_GCsxwb:b1rtxbq) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator.go#L510)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> are you duplicating the logic of `rebalanceFromConvergesOnMean` and/or `rebalanceToConvergesOnMean` here?\n</details>\nEntirely.  Cleaned up by moving the rebalancing logic into the allocator directly.  There was no need for the rangeCountBalancer\n\n---\n\n*[pkg/storage/allocator_test.go, line 202 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzo7nFBMBbCsK8RMju:-KW-T2w8G-eVgjPtzEce:bbt8k45) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator_test.go#L202)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> why is the node descriptor needed now? here and below.\n</details>\nThe ruleReplicasUniqueNodes needs it.  \n\n---\n\n*[pkg/storage/allocator_test.go, line 250 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzoscIcZGHrrgRG9Pd:-KW-Vk8XRV876PPTU6zr:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator_test.go#L250)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> This boilerplate is everywhere. How about extracting a `runToggleRuleSolver` (or perhaps a better name) helper? Ideally, you could also eliminate the named `test` closures introduced everywhere as well.\n</details>\nDone.\n\n---\n\n*[pkg/storage/allocator_test.go, line 523 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzcuYle7Qlzr3v6kPS:-KW4uc_-JPMjgiyzTMWQ:bn0y7z) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator_test.go#L523)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> +1\n</details>\nspent some time on this, reordered, named and cleaned up all the tests.\n\n---\n\n*[pkg/storage/allocator_test.go, line 1750 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzpgpcX5U2q7IS3lo7:-KW-VvCbxRIxQUOe1i9L:bux6ill) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/allocator_test.go#L1750)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> Can this share code with Example_rebalancing? The two also seem to produce different outcomes, which is worrying.\n</details>\nSure, I'll extract out the shared code.  But of course they'll produce different results, they're using a different function when choosing when to rebalance.   What matters here is that the new output looks good.\n\n---\n\n*[pkg/storage/replicate_queue.go, line 129 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzaA-xnDhyVFCEDnfi:-KW-ZrzTec8Wc-Pjd2rF:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/replicate_queue.go#L129)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> +1, log.ErrEvent probably\n</details>\nDone.\n\n---\n\n*[pkg/storage/replicate_queue.go, line 229 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzaDdM-vFiQAOxxhQj:-KW-_GccnuLpoYXo4f42:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/replicate_queue.go#L229)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> Prefer something a bit more involved:\n> ```\n> if err != nil {\n>   log.ErrEvent(ctx, err.Error()) // or log.ErrEventf(ctx, \"something something %s\", err)\n>   return nil\n> }\n> // unchanged logic below\n> if rebalanceStore == nil {\n>   ...\n>   return nil\n> }\n> ```\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 44 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzgT_iZQYLWoUzmc-P:-KW-gU9f4bTHUcotZ1oe:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L44)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this is a comment for `run`, not `rule`\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 53 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzb6YyWq7Wdi3Xy8sQ:-KW-g_Gn7Gg9erSBSIjE:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L53)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this variable is package-scoped, not file-scoped. let's give it a name that is more reflective of where it's meant to be used, like `defaultSolverRules`.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 73 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzb2rWJaaZ_wG-yR4I:-KW-gbllw_zTRBCkksoZ:bn76wzq) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L73)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> remove this method? not sure i see the point.\n</details>\nGoing to leave this for now, it's in the follow up issue list already.\n\n---\n\n*[pkg/storage/rule_solver.go, line 78 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzgqzrAEP37VIzXF2A:-KW-gkaA2_pxdt6zGuxl:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L78)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/intense/expensive/\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 91 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzgm117LUG8F-e005D:-KW-hTBDw8FhZyP3nB-a:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L91)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this comment is completely incorrect.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 114 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzgw_RB8mFqwz5DqHM:-KW-hhFnfyXWlvUllK-7:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L114)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/store/state/. what is \"candidacy information\"?\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 123 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzh2Ph7OHXKMxVUi-X:-KW-lHhZU1pWC8pMqSIZ:bsmw3ch) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L123)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> what is this?\n</details>\nWow.  Tracked this down.  It's due to not handling divided by zero errors in the rules correctly.  That was some lazy coding.  Fixed.\n\n---\n\n*[pkg/storage/rule_solver.go, line 130 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzh56PDnKF_7kl0KMi:-KW-mWjbuXlyusMVsny6:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L130)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> please rewrite this comment in terms of arguments and return values. \"ensures\" implies side effects, which this does not have.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 144 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVziKdgP134aNIwBNKD:-KW-mg_0Wj7t23-fNEPw:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L144)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this variable is useless\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 163 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVziSQfKKvp333Z1rNj:-KW-mw0cyHBWWfCdfgPS:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L163)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> again, this doesn't \"enforce\" anything, it has an argument and return values.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 169 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzhzqndgytRwJtVx0Y:-KW-n3piH6a_77J1ovEz:bm6mglk) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L169)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> `switch c.Type`?\n</details>\nnot with the anded bools\n\n---\n\n*[pkg/storage/rule_solver.go, line 183 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVziP32zk8-2GUfOty1:-KW-nQMbMyDObNFsXTaC:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L183)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> again, this doesn't \"ensure\" anything, it has an argument and return values.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 204 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVziVm9XUHCKgWMXqj9:-KW-oFJu9rzwWIAP8gnB:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L204)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> ditto\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 215 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzivCgK0z6dIyIL__a:-KW1Uda9JLTs1ezPFw6O:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L215)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> how is it organized? this returns a slice, so that's not at all clear from this comment.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 232 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzlOKLxJRa20pIIWzX:-KW1ToFBmGrqW0Vub-HB:b-2nxj8r) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L232)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> seems like there's nothing to do if this condition is false; the empty key is never used:\n> ```\n> if i < len(store.Node.Locality.Tiers) {\n>   key := store.Node.Locality.Tiers[i].Key\n>   counts[key]++\n>   if counts[key] > counts[maxKey] {\n>     maxKey = key\n>   }\n> }\n> ```\n> \n> In other words, this currently counts the occurrences of the empty key unnecessarily.\n</details>\nThat's by design.  If most of the nodes don't have a tier, we don't add it.  But we can short circuit the loop if that's the case.\n\n---\n\n*[pkg/storage/rule_solver.go, line 248 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzkd2IOXj2CEelHllj:-KW-oVpR1eu9wLNCxrhD:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L248)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> the tone of this comment is inconsistent with our style. In general, we avoid pronouns other than \"we\".\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver.go, line 262 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzmF-i-EY17uaAGE9l:-KW-oiD6jYlmHe3xlv6Z:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver.go#L262)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> \"for candidate slices\" is clear from the type. what's not clear (and surprising) is that this is a descending sort.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 40 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzmcD_jN6EgxHTnk_8:-KW1Ulvtk4sGsSymvGkl:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L40)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> useless comment\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 48 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVzmoVVYd_xhW6nZCJC:-KW3g5iLDSxi0Y8dujd8:b-mv4gym) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L48)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> am i misunderstanding this comment? seems like there are 4 \"alive\" replicas and one dead. However, this comment is also not helpful.\n</details>\nnope, you're right, the comment was wrong.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 52 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVznAddbk4rkNNu9WgA:-KW3igKQWb2FqMgSuhez:bf7056d) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L52)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> there are a _lot_ of magic strings and integers here. either sprinkle comments, or extract some local variables, or both.\n</details>\ntried to clean up a good chunk of this test.  I hope it reads cleaner.  I think I'll leave a todo to split this test up.  Each rule should have it's own set of tests, and these tests are nowhere near exhaustive and don't ensure the scores are correct.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 103 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVznKqDPPFjvFjNtYxb:-KW3mwB8G1zqafrpxsqX:b-fod7q7) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L103)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> optional: you could make these comments into a \"name\" field on the testCase struct above, and then use that as the name of a sub-test for each case.\n</details>\nFunny, I already did just that while cleaning up the test before even getting to this comment.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 238 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVznc8EOLJK3Eggpv-I:-KW3y9LJ0mW3JER-MIvN:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L238)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> if you use sub-tests, remember to remove the integer from this message and the one below.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 253 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVznmQTVZ_JcGqVXCwj:-KW4j7d4mukcO-o3JxWY:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L253)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> \"stores\"? that's not an OK name.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 326 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVznivHFEqsEKRIdP8O:-KW4j9B1YCzeIVNaYPql:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L326)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this is not a legal way to construct a store list in general, and we shouldn't use it.\n</details>\nDone.\n\n---\n\n*[pkg/storage/rule_solver_test.go, line 334 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-KVznwcbtYgTjSi9ILwg:-KW4kF1nrbEGGqyN-8Xa:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/581c2bc666c8be380221dfdfd2e66f3deff29965/pkg/storage/rule_solver_test.go#L334)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> same comment about descriptive names for sub-tests (currently the test cases are inscrutable) and removing the index if you do so.\n</details>\nDone.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/10507#-:-KW6_SPRkmm2gYlP57n_:b-u4you3)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"BramGruneir"}}