{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16809","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16809/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16809/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16809/events","html_url":"https://github.com/cockroachdb/cockroach/pull/16809","id":239693103,"number":16809,"title":"[wip,dnm] storage: introduced dedicated raft storage","user":{"login":"irfansharif","id":10536690,"avatar_url":"https://avatars0.githubusercontent.com/u/10536690?v=3","gravatar_id":"","url":"https://api.github.com/users/irfansharif","html_url":"https://github.com/irfansharif","followers_url":"https://api.github.com/users/irfansharif/followers","following_url":"https://api.github.com/users/irfansharif/following{/other_user}","gists_url":"https://api.github.com/users/irfansharif/gists{/gist_id}","starred_url":"https://api.github.com/users/irfansharif/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/irfansharif/subscriptions","organizations_url":"https://api.github.com/users/irfansharif/orgs","repos_url":"https://api.github.com/users/irfansharif/repos","events_url":"https://api.github.com/users/irfansharif/events{/privacy}","received_events_url":"https://api.github.com/users/irfansharif/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2017-06-30T06:57:54Z","updated_at":"2017-07-03T18:15:42Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/16809","html_url":"https://github.com/cockroachdb/cockroach/pull/16809","diff_url":"https://github.com/cockroachdb/cockroach/pull/16809.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/16809.patch"},"body":"Implements #16361.\r\n\r\nThis is a breaking change. To see why consider that prior to this we\r\nstored all consensus data in addition to all system metadata and user\r\nlevel keys in the same, single RocksDB instance. Here we introduce a\r\nseparate, dedicated instance for raft data (log entries and\r\n`HardState`). Cockroach nodes simply restarting with these changes, unless\r\nmigrated properly, will fail to find the most recent raft long entries\r\nand `HardState` data in the new RocksDB instance.\r\n\r\nAlso consider a cluster running mixed versions (nodes with dedicated\r\nraft storage and nodes without), what would the communication between\r\nnodes here like in light of proposer evaluated\r\nKV? Current we propagate a `storagebase.WriteBatch` through raft\r\ncontaining a serialized representation of a RocksDB write batch, this\r\nmodels the changes to be made to the single underlying RocksDB instance.\r\nFor log truncation requests where we delete log entries and/or admin\r\nsplits where we write initial `HardState` for newly formed replicas, we\r\nneed to similarly propagate a write batch (through raft) addressing the\r\nnew RocksDB instance (if the recipient node is one with these changes)\r\nor the original RocksDB instance (if the recipient node is one without\r\nthese changes). What if an older version node is the raft leader and is\r\ntherefore the one upstream of raft, propagating `storagebase.WriteBatch`es\r\nwith raft data changes but addressed to the original RocksDB instance?\r\nWhat would rollbacks look like?\r\n\r\nTo this end we introduce three modes of operation,\r\n`{Disabled,Transitioning,Enabled}RaftStorage`. We've made it so that it is\r\nsafe to transition between `DisabledRaftStorage` to\r\n`TransitioningRaftStorage`, from `TransitioningRaftStorage` to\r\n`EnabledRaftStorage` and the reverse for rollbacks. Transition from one\r\nmode to the next will take place when all the nodes in the cluster are\r\non the same previous mode.  The operation mode is set by an env var\r\n`COCKROACH_DEDICATED_RAFT_STORAGE={DISABLED,TRANSITIONING,ENABLED}`\r\n\r\n- `DisabledRaftStorage` mode preserves the previous behavior in that we use\r\n  a single RocksDB instance for both raft and user-level KV data.\r\n- `EnabledRaftStorage` mode enables the use of the dedicated RocksDB\r\n  instance for raft data. Raft log entries and the HardState are stored on\r\n  this instance alone.\r\n- `TransitioningRaftStorage` mode uses both RocksDB instances for raft data\r\n  interoperably, the raft specific and the regular instance. We use\r\n  this mode to facilitate rolling upgrades.\r\n\r\nMost of this commit is careful plumbing of an extra\r\n`engine.{Engine,Batch,Reader,Writer,ReadWriter}` for whenever we need to\r\ninteract with the new RocksDB instance. In `DisabledRaftStorage` both\r\nthese instances refer to the same underlying engine (thus preserving the\r\nprevious behaviour). The following pattern is oft repeated:\r\n\r\n```go\r\n  batch := ...\r\n  raftBatch := batch\r\n  if TransitioningRaftStorage || EnabledRaftStorage {\r\n    raftBatch = ...\r\n  }\r\n```\r\n\r\nHere are some initial performance numbers:\r\n\r\n```sh\r\n  ~ benchstat perf-disabled perf-enabled\r\n    name                             old time/op  new time/op  delta\r\n    ReplicaRaftStorage/vs=1024-4      320µs ± 6%   385µs ±18%  +20.29%  (p=0.000 n=10+10)\r\n    ReplicaRaftStorage/vs=4096-4      613µs ±14%   580µs ± 2%     ~     (p=0.278 n=10+9)\r\n    ReplicaRaftStorage/vs=16384-4    2.59ms ± 3%  2.05ms ± 4%  -20.87%  (p=0.000 n=10+9)\r\n    ReplicaRaftStorage/vs=65536-4    4.11ms ± 7%  3.29ms ± 3%  -19.97%  (p=0.000 n=10+10)\r\n    ReplicaRaftStorage/vs=262144-4   13.4ms ± 8%  10.7ms ± 3%  -20.39%  (p=0.000 n=10+10)\r\n    ReplicaRaftStorage/vs=1048576-4  56.8ms ± 3%  36.4ms ± 2%  -35.91%  (p=0.000 n=10+10)\r\n```\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/312707565","html_url":"https://github.com/cockroachdb/cockroach/pull/16809#issuecomment-312707565","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16809","id":312707565,"user":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars2.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},"created_at":"2017-07-03T18:15:42Z","updated_at":"2017-07-03T18:15:42Z","body":"> but I worry that it will be tricky to implement and under-tested (once you've transitioned to the \"disabled\" state, when does it become safe to do the downgrade?)\n\nCan't we make this downgrade path safe indefinitely? When a binary starts up in `transitioning`, it writes from its new Raft storage back to the old one if the new one is \"ahead\" (and vice versa), and in running operation maintains both. What's not clear to me is what the update would look like from a user's perspective and how they could get it wrong. Naively, one would think they'd do\n\n- copy the new binaries\n- run the new binaries (which automatically puts them in transitioning state). Everything works, but it's slower because it maintains both engines. Periodically warns about the transitioning state and the need for a further next step.\n- rolling restart, moving each binary to the full enabled state\n- the env variable becomes obsolete in the next release. We somehow make sure that you can't skip over the release that introduced the flag.\n\nPerhaps instead, we should require the explicit TRANSITIONING mode flag so that the extra step removes the env var (that will soon become irrelevant) again.\n\nDowngrades work the opposite - set transitioning flag, rolling restart, copy old binaries, rolling restart. Does seem testable enough to me with a handful of acceptance tests.\n\nOr am I completely misunderstanding how the up/downgrade process is envisioned?\n\n> I wonder if it would be better to encapsulate most of this at the Engine level: instead of two Engines, we could have a new implementation of the Engine interface that wraps two other Engines and switches between them based on the keys it is given.\n\nMaybe my opinion will change as I actually review the code, but on an abstract level I appreciate the explicitness the as-is approach brings.\n\nEdit: OK, reviewed the change and the impression remains.\n\n---\n\nReviewed 29 of 29 files at r1.\nReview status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.\n\n---\n\n*[pkg/server/config.go, line 526 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8NWQFTvhQ57NK2Jwi:-Ko8NWQFTvhQ57NK2Jwj:b-5tn8tc) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/server/config.go#L526)):*\n> ```Go\n> \tengines = nil\n> \traftEnginesCopy := raftEngines\n> \traftEngines = nil\n> ```\n\nWhile you're here, add a comment on this `nil` dance and how it ties in with the `defer` at the beginning of the method.\n\n---\n\n*[pkg/server/config.go, line 411 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko3ouAQ9MW2PnHoDoCi:-Ko8Nl8gFUaU694QcHQ6:b-bx1p6w) ([raw file](https://github.com/cockroachdb/cockroach/blob/1fc847963b49306c3ffe795ccd9df5a37505648b/pkg/server/config.go#L411)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary><blockquote>\n\nThis repeated expression should perhaps be encapsulated in some global function. \n</blockquote></details>\n\ncan't this be unconditional? If there aren't any raft engines, it would be a no-op.\n\n---\n\n*[pkg/server/config_test.go, line 45 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8NgAwXUL9pslBpMmJ:-Ko8NgAwXUL9pslBpMmK:b-bx1p6w) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/server/config_test.go#L45)):*\n> ```Go\n> \t}\n> \tif storage.TransitioningRaftStorage || storage.EnabledRaftStorage {\n> \t\tdefer raftEngines.Close()\n> ```\n\ncan't this be unconditional? If there aren't any raft engines, it would be a no-op.\n\n---\n\n*[pkg/server/config_test.go, line 69 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8Nnrcsq42FLFOOQ1G:-Ko8Nnrcsq42FLFOOQ1H:b-rqxvo7) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/server/config_test.go#L69)):*\n> ```Go\n> \t}\n> \tdefer engines.Close()\n> \tif storage.TransitioningRaftStorage || storage.EnabledRaftStorage {\n> ```\n\nditto.\n\n---\n\n*[pkg/server/node.go, line 387 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8NvfWIzEw5w6qdeHm:-Ko8NvfXTbfx3FmyLLo1:b-kkfvgu) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/server/node.go#L387)):*\n> ```Go\n> \tn.startGossip(n.stopper)\n> \n> \tlog.Infof(ctx, \"%s: started with %v engine(s), %v raft engines and attributes %v\", n, engines, raftEngines, attrs.Attrs)\n> ```\n\ns/engines/engine(s)/\n\n---\n\n*[pkg/server/server.go, line 683 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko3p4TIswDW6NZ56f5H:-Ko8OO3_PwhDQBAG7cN5:b4c9lyr) ([raw file](https://github.com/cockroachdb/cockroach/blob/1fc847963b49306c3ffe795ccd9df5a37505648b/pkg/server/server.go#L683)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary><blockquote>\n\nRefer to the configuration variables as little as possible. Here, for example, you should just close `s.raftEngines` unconditionally. \n\nAs a general rule I'd suggest creating the raft engine (and maybe batches) unconditionally, and the current transition state would only be used to A) decide which batch to write to and B) to skip operations that would impact the overall performance (like committing the write batch). \n</blockquote></details>\n\n:thumbsup: \n\n---\n\n*[pkg/storage/replica.go, line 4251 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8POmCyIeBvnBeQpSM:-Ko8POmCyIeBvnBeQpSN:b-qflpvu) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica.go#L4251)):*\n> ```Go\n> \n> \t\tif TransitioningRaftStorage || EnabledRaftStorage {\n> \t\t\t// TODO(irfansharif): Is it ever the case that we have an empty\n> ```\n\nWouldn't that be the common case? Might be missing something, but my intuition is that RaftData would only be nontrivial for special commands like ChangeReplica, Split, etc.\nClarifying comment would help.\n\n---\n\n*[pkg/storage/replica.go, line 4258 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8Pt16VVk3XqfCWXim:-Ko8Pt16VVk3XqfCWXin:b-iwqn9h) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica.go#L4258)):*\n> ```Go\n> \t\t\t// we're in TransitioningRaftStorage mode we should ensure that\n> \t\t\t// data (log entries and HardState) is copied over to the new\n> \t\t\t// engine.\n> ```\n\nThis is where Ben's suggestion looks like it would be more straightforward, but for it to work we'd need to make `ApplyBatchRepr` do the multiplexing into two underlying engines as well, which is more trouble than it's worth. Doing this explicitly looks like the way to go: I think it's fairly straightforward (since the condition on keys should be just matching a small set of prefixes) to make a command that takes a `WriteBatch` and extracts from it two write batches belonging to the regular and Raft engine, respectively.\n\n---\n\n*[pkg/storage/replica.go, line 4300 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8R4YCdRVbIlWv37he:-Ko8R4YCdRVbIlWv37hf:b-b411ym) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica.go#L4300)):*\n> ```Go\n> \n> \tstart := timeutil.Now()\n> \tisLogTruncationRequest := rResult.RaftLogDelta != nil\n> ```\n\nAdd a comment.\n\n---\n\n*[pkg/storage/replica_command.go, line 3347 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8THO0955A3Rr1tcEl:-Ko8THO0955A3Rr1tcEm:bwyoq72) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_command.go#L3347)):*\n> ```Go\n> \tif TransitioningRaftStorage || EnabledRaftStorage {\n> \t\tlocalRangeIDUnreplicatedPrefix := keys.MakeRangeIDUnreplicatedPrefix(rightRangeID)\n> \t\tif _, _, _, err := engine.MVCCDeleteRange(ctx, raftBatch, nil,\n> ```\n\nYou could be more specific here by using (basically) `makeRaftEngineKeyRanges`). Not sure if it's worth it, but would be more explicit about what's actually being removed.\n\n---\n\n*[pkg/storage/replica_data_iter.go, line 56 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8T2OPqNgl_PoRbPq8:-Ko8T2OPqNgl_PoRbPq9:bo3q993) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_data_iter.go#L56)):*\n> ```Go\n> // makeRaftEngineKeyRanges returns two key ranges, one for the HardState and\n> // one for the raft log entries associated for the given range descriptor.\n> func makeRaftEngineKeyRanges(d *roachpb.RangeDescriptor) []keyRange {\n> ```\n\nOnly needs a `RangeID`.\n\n---\n\n*[pkg/storage/replica_raftstorage.go, line 64 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8TYVBwva5OmTaH4li:-Ko8TYVBwva5OmTaH4lj:b-rfa605) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_raftstorage.go#L64)):*\n> ```Go\n> \tctx := r.AnnotateCtx(context.TODO())\n> \t// For uninitialized ranges, membership is unknown at this point.\n> \ths, err := r.mu.stateLoader.loadHardState(ctx, r.store.RaftEngine())\n> ```\n\nMoving the comment one line up seems spurious. The comment refers to the return statement (perhaps just move it there).\n\n---\n\n*[pkg/storage/replica_raftstorage.go, line 544 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8Wk8SgP6tftPAeHTV:-Ko8Wk8TnwP-Zprd0d0k:b-uylv8q) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_raftstorage.go#L544)):*\n> ```Go\n> \t\t}\n> \t\tif TransitioningRaftStorage {\n> \t\t\terr := engine.MVCCDelete(ctx, batch, nil, r.raftMu.stateLoader.RaftLogKey(i),\n> ```\n\nYour usual stats comment is missing.\n\n---\n\n*[pkg/storage/replica_raftstorage.go, line 638 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8WwDURKSdMZz_ypD6:-Ko8WwDURKSdMZz_ypD7:b-2p30wg) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_raftstorage.go#L638)):*\n> ```Go\n> \n> \t\tfor _, keyRange := range makeRaftEngineKeyRanges(desc) {\n> \t\t\t// The metadata ranges have a relatively small number of keys making usage\n> ```\n\nLooks like this comment was left accidentally.\n\n---\n\n*[pkg/storage/replica_raftstorage.go, line 166 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko3uVjhWwyTMHZWkuuC:-Ko8UNZ8xsoM5pkFLl8W:b8rpplq) ([raw file](https://github.com/cockroachdb/cockroach/blob/1fc847963b49306c3ffe795ccd9df5a37505648b/pkg/storage/replica_raftstorage.go#L166)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary><blockquote>\n\nYes, it sounds like a good idea to move the last index to the raft engine. \n</blockquote></details>\n\n:thumbsup: Also seems like this change is the one to do it.\n\n---\n\n*[pkg/storage/replica_raftstorage.go, line 180 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko3u_CIf_2xfMQiI60G:-Ko8VGiPBV0jDTJDnKfx:b-hupacz) ([raw file](https://github.com/cockroachdb/cockroach/blob/1fc847963b49306c3ffe795ccd9df5a37505648b/pkg/storage/replica_raftstorage.go#L180)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary><blockquote>\n\nI'm less sure about this one but it's probably a good idea to move TruncatedState too. \n</blockquote></details>\n\nThat would be good in order to facilitate faster log truncation, too. We currently shove all of that through Raft, but there's really no good reason afaict. The lease holder could just truncate its logs when it feels like it and let the followers know what to truncate to outside of Raft. If this PR were able to make the truncated index unreplicated (enough) to do this as a follow-up, that would be good; the transitioning state is a good opportunity for these kinds of things. Have to think it through, though.\n\n---\n\n*[pkg/storage/replica_raftstorage_test.go, line 116 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8Xrmcn_SJKoc30hrG:-Ko8Xrmcn_SJKoc30hrH:b-rnj2xh) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_raftstorage_test.go#L116)):*\n> ```Go\n> }\n> \n> func BenchmarkReplicaRaftStorage(b *testing.B) {\n> ```\n\nThe benchmark name is pretty generic. Wouldn't hurt to mention you're doing serial puts; it's arguable whether this benchmark is really specific to raft storage.\n\nBut this makes me think: when in the new mode, what do all the other benchmarks run with? Will they need updates as well to run with \"realistic\" Raft engines, too?\n\n---\n\n*[pkg/storage/replica_raftstorage_test.go, line 118 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8Xd1pnLYlVG1sLGXy:-Ko8Xd1pnLYlVG1sLGXz:bwsfyln) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/replica_raftstorage_test.go#L118)):*\n> ```Go\n> func BenchmarkReplicaRaftStorage(b *testing.B) {\n> \tfor _, valueSize := range []int{1 << 10, 1 << 12, 1 << 14, 1 << 16, 1 << 18, 1 << 20} {\n> \t\tb.Run(fmt.Sprintf(\"vs=%d\", valueSize), func(b *testing.B) {\n> ```\n\nnit: `s/vs/valSize/`.\n\n---\n\n*[pkg/storage/store.go, line 137 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8Z3sJAPraHYU45pUf:-Ko8Z3sJAPraHYU45pUg:b-utlrd6) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/store.go#L137)):*\n> ```Go\n> // Consider the example where we introduced a dedicated RocksDB instance for\n> // raft data where the following modes are used. Briefly, the major version\n> // with this feature stored raft data (log entries and raft HardState) in a\n> ```\n\nnit: s/stored/stores/\n\ns/log entries/such as log entries/ (anticipating that we might add lastIndex, and perhaps truncated state)\n\n---\n\n*[pkg/storage/store.go, line 178 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8_nDOi_V4cYct3FCf:-Ko8_nDOi_V4cYct3FCg:b7dq34n) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/store.go#L178)):*\n> ```Go\n> //   by copying over all existing raft data (log entries + HardState) into the new\n> //   dedicated raft engine\n> // - Nodes will be restarted to run in this mode, they will be able to\n> ```\n\nGrammar is off throughout this sentence.\n\n---\n\n*[pkg/storage/store.go, line 1256 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8an2bX6umItC3dgJx:-Ko8an2bX6umItC3dgJy:bg4gs66) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/store.go#L1256)):*\n> ```Go\n> \t\t\t// TODO(irfansharif): Will need to copy over hard state + log\n> \t\t\t// entries for each range if running in transitioning mode and we\n> \t\t\t// were on an old cockroach version before.\n> ```\n\nOr if we were on a new version before, need to copy from dedicated engine to base engine.\n\n---\n\n*[pkg/storage/store.go, line 2428 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8b3gLNPa5SNwuh2SC:-Ko8b3gLNPa5SNwuh2SD:bnqiu40) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/store.go#L2428)):*\n> ```Go\n> \n> \t\tcapacity.Capacity += raftEngCapacity.Capacity\n> \t\tcapacity.Available += raftEngCapacity.Available\n> ```\n\nThis isn't really right, but probably the most straightforward way to handle it for now. Should make sure it isn't getting lost, though. Add a TODO?\n\n---\n\n*[pkg/storage/store.go, line 158 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko3w1Pp2Piuv98TtmOL:-Ko8ZLEiY6tyqnfbHo2P:byirnne) ([raw file](https://github.com/cockroachdb/cockroach/blob/1fc847963b49306c3ffe795ccd9df5a37505648b/pkg/storage/store.go#L158)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary><blockquote>\n\nYes, this should be a cluster setting (which means that the global variables will have to be changed to functions). \n</blockquote></details>\n\nWhat is your plan with the cluster setting, Ben? Nodes will need to rewrite on-disk data which is hard to do once they're running. Would the idea be that nodes store the \"last\" cluster setting they've seen and apply it on restart? It's tricky to guarantee that all nodes have seen the latest setting, a problem you don't have with an env var. We could provide a helper - `./cockroach migrate raft.dedicated_storage='transitioning'` which would set the env var, wait for it to have propagated, and then prompts the user to do a rolling restart after which they run `./cockroach migrate raft.dedicated_storage='enabled'` and get another set of instructions and a round of rolling restart. Or what were you envisioning? I find the lack of explicit control troubling as it increases the chance to find yourself in unscripted situations. Then again, setting env vars may not be the best solution for some deployments.\n\n---\n\n*[pkg/storage/engine/rocksdb.go, line 324 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-Ko8b_HN8LMWWbseCHp8:-Ko8b_HN8LMWWbseCHp9:b-7fnfgo) ([raw file](https://github.com/cockroachdb/cockroach/blob/294ed4d8896627f25691fcfb2fee1c8caf832565/pkg/storage/engine/rocksdb.go#L324)):*\n> ```Go\n> // NewTestRocksDB allocates and returns an on-disk RocksDB instance. The\n> // caller must call the engine's Close method when the engine is no longer\n> // needed which will delete the files stored on disk.\n> ```\n\nI'm always wary of this pattern. If you pass in a `dir`, I think the caller should have to clean up the directory. Alternatively, the caller can pass an existing dir and the engine promises to remove all its files when it's closed (i.e. you'd just make a tempdir inside of dir and rmdir that at the end).\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/16809#-:-Ko8K9_jXqRK5DcMau_n:b8vmx83)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tschottdorf"}}