This will probably need a quick rebase since #4308 merged

Review status: 0 of 11 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io

This PR also has a big impact on the {Update,Delete}1 benchmarks. Presumably they were also encountering the same performance degradation.

name                   old time/op    new time/op    delta
Insert1_Cockroach-8       312µs ± 1%     317µs ± 2%   +1.58%         (p=0.001 n=9+10)
Insert10_Cockroach-8      597µs ± 2%     590µs ± 2%   -1.07%        (p=0.043 n=10+10)
Insert100_Cockroach-8    3.14ms ± 1%    3.06ms ± 1%   -2.57%         (p=0.000 n=9+10)
Update1_Cockroach-8       752µs ± 1%     602µs ± 1%  -19.92%        (p=0.000 n=10+10)
Update10_Cockroach-8     3.51ms ± 1%    3.55ms ± 1%   +1.00%        (p=0.000 n=10+10)
Update100_Cockroach-8    31.0ms ± 1%    31.6ms ± 1%   +1.93%         (p=0.000 n=10+8)
Delete1_Cockroach-8      1.04ms ±13%    0.61ms ± 1%  -40.70%         (p=0.000 n=10+9)
Delete10_Cockroach-8     1.50ms ± 9%    1.26ms ± 1%  -16.40%        (p=0.000 n=10+10)
Delete100_Cockroach-8    6.46ms ± 1%    6.61ms ± 2%   +2.29%         (p=0.000 n=9+10)
Scan1_Cockroach-8         135µs ± 1%     135µs ± 1%     ~           (p=0.315 n=10+10)
Scan10_Cockroach-8        166µs ± 2%     167µs ± 1%   +0.82%         (p=0.013 n=10+9)
Scan100_Cockroach-8       421µs ± 2%     431µs ± 2%   +2.40%        (p=0.005 n=10+10)

Not sure what is up with the circleci failure. I did notice that an unrelated changed had snuck into my GLOCKFILE. Perhaps that was it.

Anyone want to stamp this with an LGTM?

LGTM. I hope you've seen Igor's comment which was on a commit that you've since replaced.

Reviewed 4 of 11 files at r1, 6 of 7 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

storage/engine/rocksdb_test.go, line 614 [r1] (raw file):
Still feels funny that this worked. Put a TODO for me please (I'm assuming you don't want to take 5 and figure it out).

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 1 unresolved discussion.

storage/engine/rocksdb_test.go, line 614 [r1] (raw file):
The txn parameter is ignored if the timestamp is "zero". Changing to a non-zero timestamp and we see the error you expect.

Comments from the review on Reviewable.io

Gotcha. I may rest in piece. Thanks for looking.

On Thu, Feb 11, 2016 at 1:57 PM Peter Mattis notifications@github.com
wrote:

Review status: all files reviewed at latest revision, 1 unresolved

discussion.

storage/engine/rocksdb_test.go, line 614 [r1]
https://reviewable.io:443/reviews/cockroachdb/cockroach/4319#-KAFjXC4YePWHJSqPgEf-r1-614:-KAGfHbe0bmeR7BQb7fK:2016351573
(raw file

cockroach/storage/engine/rocksdb_test.go

Line 614 in fd06855

if err := MVCCPut(rocksdb, nil, key, zeroTS, value, txn1); err != nil {

):
The txn parameter is ignored if the timestamp is "zero". Changing to a

non-zero timestamp and we see the error you expect.

Comments from the review on Reviewable.io
https://reviewable.io:443/reviews/cockroachdb/cockroach/4319

—

Reply to this email directly or view it on GitHub
#4319 (comment)
.

The unrelated GLOCKFILE change is because the docker API has a dependency hidden behind a build tag that glock save can't see (#4282). For now it is only safe to run glock save from linux (build/builder.sh glock save will work); if you run it from a mac you'll have to manually tweak the output.

See robfig/glock#29