storage.engine: add version file to store directory

[BramGruneir]

This adds a new file VERSION to all store directories. To facilitate this, I've
added a new proto to keep track the format. This has a tiny bit of future
proofing to allow us to have multiple database types with different data
versions as well as a minimum value we support.

When needed, having this file will allow us to automatically perform migrations
during the call to rocksDB.Open().

Fixes #2718.

---

This change is

[tamird]

Note that this will likely have a write skew anomaly with #5694.

[petermattis]

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.

---

storage/engine/version.proto, line 21 [r1] (raw file):
s/MVCCMetadata/Version/g

Do you think a proto is overkill here? I was imagining a text file containing the version number so that it was human readable.

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.

---

storage/engine/version.proto, line 21 [r1] (raw file):
I can convert it to just a go struct. I just kind of like the security of a proto.
Also, the file is human readable. It's json.

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

storage/engine/version.proto, line 21 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 0 of 4 files reviewed at latest revision, 7 unresolved discussions.

---

storage/engine/version.go, line 26 [r3] (raw file):
s/VERSION/COCKROACHDB_VERSION/ to guard against e.g. rocksdb introducing its own version file.

---

storage/engine/version.go, line 29 [r3] (raw file):
versionRocksDBMinimum should be set to versionNoFile, so you don't need to special case versionNoFile when loading. Then writing the first version file can simply be the migration from versionNoFile to versionRocksDBCurrent.

---

storage/engine/version.go, line 35 [r3] (raw file):
In the event that we move away from rocksdb, the version file will parse successfully as if it had a rocksdb version of zero. This seems like an invitation for special-cases and bugs, so I'd rather have one top-level version field that we'll be able to continue using no matter what changes we make. We can always introduce more detailed version information when/if we need it.

---

storage/engine/version.go, line 43 [r3] (raw file):
Comment doesn't match code. I think it's fine to make versionNoFile zero (except for the issue in the previous comment) and then you can use Version{} instead of noVersionFile.

---

storage/engine/version.go, line 51 [r3] (raw file):
Use filepath.Join instead of manually constructing the path.

---

storage/engine/version.go, line 57 [r3] (raw file):
Why is this function name plural?

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: 0 of 4 files reviewed at latest revision, 7 unresolved discussions.

---

storage/engine/version.go, line 26 [r3] (raw file):
Done.

---

storage/engine/version.go, line 29 [r3] (raw file):
Done.

---

storage/engine/version.go, line 35 [r3] (raw file):
Done.

---

storage/engine/version.go, line 43 [r3] (raw file):
Done.

---

storage/engine/version.go, line 51 [r3] (raw file):
Done.

---

storage/engine/version.go, line 57 [r3] (raw file):
Fixed

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 19 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
why the ver < versionCurrent check? just to avoid the no-op case? i'd remove this.

---

storage/engine/rocksdb_test.go, line 175 [r4] (raw file):
the what?

---

storage/engine/rocksdb_test.go, line 200 [r4] (raw file):
i know it's copied, but the 512<<20 and minmemtablebudget could both be zero, this is just misdirection currently

---

storage/engine/rocksdb_test.go, line 202 [r4] (raw file):
why is this deferred?

---

storage/engine/version.go, line 29 [r3] (raw file):
why not set it to versionNoFile rather than 0?

---

storage/engine/version.go, line 34 [r4] (raw file):
what's the intention with using json here? if it's for the future flexibility of adding more fields, we should use a proto? if not, just strconv.FormatInt?

---

storage/engine/version.go, line 35 [r4] (raw file):
pretty sure this json struct tag does nothing

---

storage/engine/version.go, line 44 [r4] (raw file):
comment wrong

---

storage/engine/version.go, line 52 [r4] (raw file):
s/0/versionNoFile/

---

storage/engine/version.go, line 66 [r4] (raw file):
how can this happen?

---

storage/engine/version.go, line 69 [r4] (raw file):
not necessary - ioutil.WriteFile: WriteFile writes data to a file named by filename. If the file does not exist, WriteFile creates it with permissions perm; otherwise WriteFile truncates it before writing.

---

storage/engine/version_test.go, line 70 [r4] (raw file):
prefer :=

---

storage/engine/version_test.go, line 71 [r4] (raw file):
weird line break

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: 1 of 4 files reviewed at latest revision, 19 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
If we don't have a file yet, we should write the version file info. The last thing we want to to re-write the version file each time.

---

storage/engine/rocksdb_test.go, line 175 [r4] (raw file):
removed

---

storage/engine/rocksdb_test.go, line 200 [r4] (raw file):
If memtableBudget < minmemtablebudget, it will throw an error. You're right about the cache size, I've change it to 0.

---

storage/engine/rocksdb_test.go, line 202 [r4] (raw file):
I think I had something afterwards originally, removed the defer.

---

storage/engine/version.go, line 29 [r3] (raw file):
Done.

---

storage/engine/version.go, line 34 [r4] (raw file):
Ok, I had this in a proto and Pete suggested that that was overkill. So I removed the proto. I don't care either way, but I do like the idea of having a format we can expand on and is human readable. So I stand by the use of JSON, but I don't care if we go proto or not.

---

storage/engine/version.go, line 35 [r4] (raw file):
I'm pretty sure you're right. Removed.

---

storage/engine/version.go, line 44 [r4] (raw file):
fixed

---

storage/engine/version.go, line 52 [r4] (raw file):
Done.

---

storage/engine/version.go, line 66 [r4] (raw file):
it shouldn't, old code, removed.

---

storage/engine/version.go, line 69 [r4] (raw file):
cool, removed

---

storage/engine/version_test.go, line 70 [r4] (raw file):
Done.

---

storage/engine/version_test.go, line 71 [r4] (raw file):
Done.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
i'm confused. what's the harm in rewriting this file each time?

---

storage/engine/rocksdb_test.go, line 202 [r4] (raw file):
heh you could

defer rocksdb.Close()
return rocksdb.Open()

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: 3 of 4 files reviewed at latest revision, 10 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
The harm is minimal, but there's no reason to do so. Why do you want to write a new file each time when the current one is sufficient?

---

storage/engine/rocksdb_test.go, line 202 [r4] (raw file):
nice :) Done.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 7 of 7 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
Because "ver < versionCurrent" wasn't obviously correct and it seems easier to remove it than to convince myself of its correctness since the outcome must be equivalent.

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
How about if I change it to the explicit case in which we have no file? ver == versionNoFile ?

---

Comments from the review on Reviewable.io

[tamird]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
seems that would be wrong when we implement the first migration.

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
Ok, I'm going to leave this as is, but I'm going to clean up the comment above it.

---

Comments from the review on Reviewable.io

[bdarnell]

LGTM

---

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

storage/engine/rocksdb.go, line 148 [r4] (raw file):
ver < versionCurrent seems clear enough to me. We should avoid rewriting the file unnecessarily.

Also note that only the most drastic migrations should actually use this mechanism, since we can't make the write of the version file atomic with rocksdb operations. We'll have a version key inside the rocksdb instance for most migrations; this file would only be for changes that affect fundamental rocksdb operation (like a custom comparator).

---

storage/engine/version.go, line 34 [r4] (raw file):
I think it's good for this to be human-readable, so JSON is better than proto here. I think a plain number would be fine too, but the flexibility doesn't hurt.

---

storage/engine/version.go, line 70 [r6] (raw file):
ioutil.WriteFile is not atomic. Add a TODO to replace this with an atomic rename before we have our first migration, since it would be bad if the version file got lost due to an ill-timed crash.

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: 2 of 4 files reviewed at latest revision, 4 unresolved discussions.

---

storage/engine/version.go, line 70 [r6] (raw file):
I've updated the code so it is now atomic.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

storage/engine/version.go, line 28 [r7] (raw file):
shouldn't this be an enumerated type? sorry for late comment.

---

Comments from the review on Reviewable.io

[BramGruneir]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

storage/engine/version.go, line 28 [r7] (raw file):
Done.

---

Comments from the review on Reviewable.io