[dnm] storage: use engine.Batch in replica read codepath by irfansharif · Pull Request #43388 · cockroachdb/cockroach

irfansharif · 2019-12-20T04:00:58Z

By making this change, I observe TestReplicaLookupUseReverseScan
failing after being stuck emitting just the following:

W191220 03:55:37.815241 21 internal/client/range_lookup.go:259  range lookup of key "c" found only non-matching ranges []; retrying
W191220 03:55:37.816682 21 internal/client/range_lookup.go:259  range lookup of key "c" found only non-matching ranges []; retrying
W191220 03:55:37.818981 21 internal/client/range_lookup.go:259  range lookup of key "c" found only non-matching ranges []; retrying
W191220 03:55:37.824012 21 internal/client/range_lookup.go:259  range lookup of key "c" found only non-matching ranges []; retrying

This does not match with my expected behavior of NewBatch. Should the
behavior not be identical to when Engine().NewReadOnly() is used to
evaluate the read only command? If no, why not?

Release note: None

cockroach-teamcity · 2019-12-20T04:01:06Z

This change is

By making this change, I observe `TestReplicaLookupUseReverseScan` failing after being stuck emitting just the following: ``` W191220 03:55:37.815241 21 internal/client/range_lookup.go:259 range lookup of key "c" found only non-matching ranges []; retrying W191220 03:55:37.816682 21 internal/client/range_lookup.go:259 range lookup of key "c" found only non-matching ranges []; retrying W191220 03:55:37.818981 21 internal/client/range_lookup.go:259 range lookup of key "c" found only non-matching ranges []; retrying W191220 03:55:37.824012 21 internal/client/range_lookup.go:259 range lookup of key "c" found only non-matching ranges []; retrying ``` This does not match with my expected behavior of NewBatch. Should the behavior not be identical to when Engine().NewReadOnly() is used to evaluate the read only command? If no, why not? Release note: None

irfansharif · 2019-12-20T04:24:40Z

Welp, it looks like a hundred other tests are also broken. I didn't realize engine.Batches didn't have a "read at snapshot of underlying engine at time of creation" guarantee.

petermattis

Welp, it looks like a hundred other tests are also broken. I didn't realize engine.Batches didn't have a "read at snapshot of underlying engine at time of creation" guarantee.

Huh. That's strange. NewReadOnly and NewBatch should have the same behavior with regards to snapshotting the underlying engine. In both cases, the snapshot does not occur until the first iterator is created.

I'm curious what is motivating this change, though. Are you attempting to get rid of NewReadOnly?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nvanbenschoten, and @petermattis)

irfansharif

I was trying to see what plumbing an engine.Batch type throughout batcheval would look like, which in turn was motivated by raft storage work where I wanted more precisely specified usage of engine.Batch vs. engine.Engine types. If this helps makes pkg/engine interfaces any smaller, I'm all for it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

petermattis

I was trying to see what plumbing an engine.Batch type throughout batcheval would look like, which in turn was motivated by raft storage work where I wanted more precisely specified usage of engine.Batch vs. engine.Engine types. If this helps makes pkg/engine interfaces any smaller, I'm all for it.

I believe we originally had an engine.Batch plumbed throughout batcheval (or the precursor to batcheval), but intentionally moved to using engine.NewReadOnly for read-only requests. It would be nice if NewReadOnly returned a Reader instead of a ReadWriter, but plumbing that through batcheval seemed onerous.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

irfansharif

Digging through history I find #16406 where we moved from using the raw engine to engine.NewReadOnly to be able to reuse iterators. (Bonus: there are benchmarks comparing engine.Batch vs engine.ReadOnly). Previous to that we've always used the raw engine for read-only requests, or at least as of 2015:

cockroach/storage/range.go

Lines 608 to 609 in 433125d

    
           // Execute read-only command. 
        
           intents, err := r.executeCmd(r.rm.Engine(), nil, args, reply)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

irfansharif · 2019-12-20T16:17:33Z

Ok so lazily stealing numbers from benchdiff:

                                          time/op (ns/op)

IterOnBatch_RocksDB/writes=10-24	      1601.5	        (p=0.002 n=10+10)
IterOnEngine_RocksDB/writes=10-24	      1950.8	        (p=0.549 n=9+10)
IterOnReadOnly_RocksDB/writes=10-24	      966.8888889	    (p=0.324 n=10+9)

IterOnBatch_RocksDB/writes=100-24	      2015.2	        (p=0.000 n=8+10)
IterOnEngine_RocksDB/writes=100-24	      2110.7	        (p=0.617 n=10+10)
IterOnReadOnly_RocksDB/writes=100-24	  1163.444444	    (p=0.764 n=10+9)

IterOnBatch_RocksDB/writes=1000-24	      2618.8	        (p=0.211 n=10+10)
IterOnEngine_RocksDB/writes=1000-24	      2355	            (p=0.912 n=10+10)
IterOnReadOnly_RocksDB/writes=1000-24	  1385.888889	    (p=0.324 n=10+9)

IterOnBatch_RocksDB/writes=10000-24	      3338.1	        (p=0.579 n=10+10)
IterOnEngine_RocksDB/writes=10000-24	  2570.1	        (p=0.278 n=9+10)
IterOnReadOnly_RocksDB/writes=10000-24	  1593.8	        (p=0.566 n=10+10)

The overhead in reading through engine.Batch vs. engine.NewReadOnly is is not as small as I thought it would be (correct me if I'm wrong). I'll abandon this now.

irfansharif · 2019-12-20T16:19:47Z

I didn't realize engine.Batches didn't have a "read at snapshot of underlying engine at time of creation" guarantee.

I'm still surprised this is a thing, and it sounds like it should not be? I can file separately if storage is interested in digging into it.

petermattis · 2019-12-20T16:26:05Z

I'm still surprised this is a thing, and it sounds like it should not be? I can file separately if storage is interested in digging into it.

Are there any bugs due to this? I would be interested in understanding why replacing engine.NewReadOnly with engine.NewBatch caused test failures (they should be equivalent for read-only operations), but changing the semantics of engine.NewBatch to take a snapshot of the underlying database is very likely to have performance implications.

nvb · 2019-12-20T18:45:05Z

It would be nice if NewReadOnly returned a Reader instead of a ReadWriter, but plumbing that through batcheval seemed onerous.

I'm pretty sure @benesch tried to do this at some point and eventually gave up. That should tell you all you need to know about how onerous this would be.

benesch · 2019-12-20T18:55:18Z

Actually IIRC I gave up because I was on my way out and ran out of time, not because it was terribly technically challenging. 😄 The PR is here: #26916. There is still some weirdness where you still have a ReadWriter when you really want a Reader, but I think the PR fixed a lot more weirdness than it created!

…

On Fri, Dec 20, 2019 at 1:45 PM Nathan VanBenschoten < ***@***.***> wrote: It would be nice if NewReadOnly returned a Reader instead of a ReadWriter, but plumbing that through batcheval seemed onerous. I'm pretty sure @benesch <https://github.com/benesch> tried to do this at some point and eventually gave up. That should tell you all you need to know about how onerous this would be. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43388?email_source=notifications&email_token=AAGXSIFQR33CSLAVMUKH3FLQZUHDFA5CNFSM4J5WXTOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHNZ4MI#issuecomment-568041009>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGXSIFTUGTQ2VHCU2WDKXLQZUHDFANCNFSM4J5WXTOA> .

…types We should be using {reader,readWriter} for {Reader,ReadWriter} respectively instead. I'd skipped everything under batcheval when working on cockroachdb#43265, back when I thought it'd be a good idea to plumb in an engine.Batch throughout replica read codepaths. Due to perf penalty as seen on cockroachdb#43388, I'm no longer going to be doing that. This is the remaining work. Release note: None

…types We should be using {reader,readWriter} for {Reader,ReadWriter} respectively instead. I'd skipped everything under `batcheval` when working on cockroachdb#43265, back when I thought it'd be a good idea to plumb in an engine.Batch throughout replica read codepaths. Due to perf penalty as seen on cockroachdb#43388, I'm no longer going to be doing that. This is the remaining work. Release note: None

@mjibson

43792: batcheval: clean up remaning variable names of `{Reader,ReadWriter}` types r=irfansharif a=irfansharif Ignore first commit, reviewed in #43764. We should be using {reader,readWriter} for {Reader,ReadWriter} respectively instead. I'd skipped everything under batcheval when working on #43265, back when I thought it'd be a good idea to plumb in an engine.Batch throughout replica read codepaths. Due to perf penalty as seen on #43388, I'm no longer going to be doing that. This is the remaining work. Release note: None 43811: hba: stricter parsing and better pretty-printing r=knz a=knz The following defects were found by @mjibson using `go-fuzz` (#43805) and are fixed by this patch: - the code would parse then fail to pretty-print an empty connection type and auth method. - the code would parse then pretty-print incorrectly a connection type or auth method containing commas or other special characters. - the code would accept an empty string as address. - the code would accept then fail to pretty-print columns containing special (control) characters. Note that the code previously supported tabs (\t) and carriage returns (\r) inside quoted strings, and this is now rejected. For most fields, this does not matter, as these special characters were not valid semantically anyway. However, for the method options in the last column it is theoretically possible for a method to wish to use such special characters. This is not currently the case for the CCL `gss` method already implemented, so this change is not introducing a regression. A comment is left in the code to indicate what needs to be changed if a future method needs this support. Release note (security): CockroachDB now properly rejects control characters in the value of the cluster setting `server.host_based_authentication.configuration`. These characters were previously accepted and would silently yield rules matching differently from intended. Deployments careful to strip control characters from their HBA configuration are not affected. Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

[dnm] storage: remove race assertions for cleaner diff

f712c40

irfansharif added the do-not-merge bors won't merge a PR with this label. label Dec 20, 2019

irfansharif requested review from nvb and petermattis December 20, 2019 04:00

irfansharif force-pushed the 191219.ro-batch-question branch from 2f44719 to 9f68d6c Compare December 20, 2019 04:01

irfansharif requested a review from ajwerner December 20, 2019 04:08

petermattis reviewed Dec 20, 2019

View reviewed changes

irfansharif commented Dec 20, 2019

View reviewed changes

petermattis reviewed Dec 20, 2019

View reviewed changes

irfansharif commented Dec 20, 2019

View reviewed changes

irfansharif closed this Dec 20, 2019

irfansharif deleted the 191219.ro-batch-question branch December 20, 2019 16:17

irfansharif mentioned this pull request Dec 20, 2019

storage: replacing engine.NewReadOnly with engine.NewBatch causes test failures #43406

Closed

irfansharif mentioned this pull request Jan 7, 2020

batcheval: clean up remaning variable names of {Reader,ReadWriter} types #43792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dnm] storage: use engine.Batch in replica read codepath #43388

[dnm] storage: use engine.Batch in replica read codepath #43388
irfansharif wants to merge 2 commits intocockroachdb:masterfrom
irfansharif:191219.ro-batch-question

irfansharif commented Dec 20, 2019

Uh oh!

cockroach-teamcity commented Dec 20, 2019

Uh oh!

irfansharif commented Dec 20, 2019

Uh oh!

petermattis left a comment

Uh oh!

irfansharif left a comment

Uh oh!

petermattis left a comment

Uh oh!

irfansharif left a comment

Uh oh!

irfansharif commented Dec 20, 2019

Uh oh!

irfansharif commented Dec 20, 2019

Uh oh!

petermattis commented Dec 20, 2019

Uh oh!

nvb commented Dec 20, 2019

Uh oh!

benesch commented Dec 20, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	// Execute read-only command.
	intents, err := r.executeCmd(r.rm.Engine(), nil, args, reply)

Conversation

irfansharif commented Dec 20, 2019

Uh oh!

cockroach-teamcity commented Dec 20, 2019

Uh oh!

irfansharif commented Dec 20, 2019

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif commented Dec 20, 2019

Uh oh!

irfansharif commented Dec 20, 2019

Uh oh!

petermattis commented Dec 20, 2019

Uh oh!

nvb commented Dec 20, 2019

Uh oh!

benesch commented Dec 20, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants