Skip to content

storage: clean up usage of {eng,batch} engine.{Reader,Writer,ReadWriter}#43265

Merged
craig[bot] merged 4 commits intocockroachdb:masterfrom
irfansharif:191217.eng-name-cleanup
Dec 18, 2019
Merged

storage: clean up usage of {eng,batch} engine.{Reader,Writer,ReadWriter}#43265
craig[bot] merged 4 commits intocockroachdb:masterfrom
irfansharif:191217.eng-name-cleanup

Conversation

@irfansharif
Copy link
Copy Markdown
Contributor

We should be using {reader,writer,readWriter} engine.{Reader,Writer,ReadWriter} instead. See individual commits for review.

@irfansharif irfansharif requested review from ajwerner and nvb December 17, 2019 22:32
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewed 3 of 3 files at r1, 6 of 6 files at r2, 4 of 4 files at r3, 5 of 5 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @irfansharif)


pkg/storage/replica_raft.go, line 1621 at r4 (raw file):

	oldTruncatedState, newTruncatedState *roachpb.RaftTruncatedState,
	loader stateloader.StateLoader,
	batch engine.Batch,

Why did you make this a batch?

@irfansharif irfansharif force-pushed the 191217.eng-name-cleanup branch from f3fa87a to 4fa2480 Compare December 18, 2019 00:52
Copy link
Copy Markdown
Contributor Author

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/storage/replica_raft.go, line 1621 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why did you make this a batch?

Woops, snuck in from another (incoming) diff. That being said, I think philosophically we should be using engine.Batch when we are, infact, using {rocksdb,pebble} batches. It may be obvious to us now, but all of batcheval/proposer kv never deal with raw engines and the function signatures should reflect as such.

Copy link
Copy Markdown
Contributor Author

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR, will merge on green (lint failures).

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

Rename "eng/batch" `engine.Writer`s for clarity. Additionally change an
unrecoverable error condition to a panic.

Release note: None
Rename "eng/batch" `engine.Reader`s for clarity.

Release note: None
Rename "eng" `engine.ReadWriter`s for clarity.

Release note: None
Rename "batch" `engine.ReadWriter`s for clarity. I've intentionally
skipped instances under `batcheval`, we should be only be accepting
engine.Batch there instead.

Release note: None
@irfansharif
Copy link
Copy Markdown
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Dec 18, 2019
43265: storage: clean up usages of `{eng,batch} engine.{Reader,Writer,ReadWriter}` r=irfansharif a=irfansharif

We should be using `{reader,writer,readWriter} engine.{Reader,Writer,ReadWriter}` instead. See individual commits for review.

Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Dec 18, 2019

Build succeeded

@craig craig bot merged commit 1485404 into cockroachdb:master Dec 18, 2019
craig bot pushed a commit that referenced this pull request Dec 18, 2019
43271: engine: clean up variable names of `{Reader,Writer,ReadWriter}` types r=irfansharif a=irfansharif

We should be using {reader,writer,readWriter} for
{Reader,Writer,ReadWriter} respectively instead.

(Same thing as #43265, but for pkg/storage/engine).

Release note: None

Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
@irfansharif irfansharif deleted the 191217.eng-name-cleanup branch December 18, 2019 03:00
@irfansharif irfansharif changed the title storage: clean up usages of {eng,batch} engine.{Reader,Writer,ReadWriter} storage: clean up usage of {eng,batch} engine.{Reader,Writer,ReadWriter} Dec 18, 2019
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Dec 19, 2019
craig bot pushed a commit that referenced this pull request Dec 19, 2019
43372: storage: rename `{e,b} engine.{Reader,Writer,ReadWriter}` r=petermattis a=irfansharif

Leftover work from #43265.

Release note: None

Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Jan 7, 2020
…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
irfansharif added a commit to irfansharif/cockroach that referenced this pull request Jan 7, 2020
…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
craig bot pushed a commit that referenced this pull request Jan 8, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants