storage/engine: extract an MVCCScanOptions struct #32389
Conversation
|
This change is |
|
There doesn't seem to be any performance to this change, mercifully: |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/storage/engine/mvcc_test.go, line 1595 at r1 (raw file):
// A scan with consistent=false should fail in a txn. if _, _, _, err := MVCCScan(context.Background(), engine, keyMin, keyMax, math.MaxInt64, hlc.Timestamp{WallTime: 1}, MVCCScanOptions{Inconsistent: !false, Txn: txn1}); err == nil {
!false??? (There are several places in this file to fix).
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/mvcc_test.go, line 1595 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
!false??? (There are several places in this file to fix).
Hah, whoops! I thought I cleaned all of those up. (I used golang.org/x/tools/cmd/eg to do this refactor automatically, and it's not smart enough to simplify booleans.)
The signatures of the functions in the MVCCScan family are extremely unwieldy. Extract an MVCCScanOptions struct for the fields that have reasonable default values. We can immediately realize the benefits of this change by removing MVCCReverseScan, MVCCReverseScanWithBytes, and MVCCIterateUsingIter, as the functionality provided by those functions can be requested with the appropriate option in the MVCCScanOptions struct. This paves the way towards introducing additional parameters, like minimum and maximum timestamp bounds. Updates cockroachdb#28358. Release note: None
benesch
force-pushed
the
scan-options
branch
from
1eccba3 to
4678a31
Compare
There was a problem hiding this comment.
bors r=petermattis
Reviewable status:
pkg/storage/engine/mvcc_test.go, line 1595 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
Hah, whoops! I thought I cleaned all of those up. (I used golang.org/x/tools/cmd/eg to do this refactor automatically, and it's not smart enough to simplify booleans.)
Done.
There was a problem hiding this comment.
Reviewable status:
Build failed |
|
Failures look spurious. bors r=petermattis |
32389: storage/engine: extract an MVCCScanOptions struct r=petermattis a=benesch The signatures of the functions in the MVCCScan family are extremely unwieldy. Extract an MVCCScanOptions struct for the fields that have reasonable default values. We can immediately realize the benefits of this change by removing MVCCReverseScan, MVCCReverseScanWithBytes, and MVCCIterateUsingIter, as the functionality provided by those functions can be requested with the appropriate option in the MVCCScanOptions struct. This paves the way towards introducing additional parameters, like minimum and maximum timestamp bounds. Updates #28358. Release note: None Co-authored-by: Nikhil Benesch <[email protected]>
Build succeeded |
32395: storage/engine: wrap long lines in tests r=petermattis a=benesch Ignore the first commit, which is landing now in #32389. This isn't a high-priority cleanup, but it's been driving me nuts and I was in the area. --- The tests in this package were a case study in how to wrap code for maximum illegibility. Enforce a line limit of 100 characters, except where exceeding the limit by a few characters makes the code substantially more readable. The primary tool employed is extracting calls to context.Background() into a per-test ctx variable. All changes in this patch are straightforward reformattings with no behavior changes. Release note: None Co-authored-by: Nikhil Benesch <[email protected]>
The signatures of the functions in the MVCCScan family are extremely
unwieldy. Extract an MVCCScanOptions struct for the fields that have
reasonable default values.
We can immediately realize the benefits of this change by removing
MVCCReverseScan, MVCCReverseScanWithBytes, and MVCCIterateUsingIter,
as the functionality provided by those functions can be requested with
the appropriate option in the MVCCScanOptions struct.
This paves the way towards introducing additional parameters, like
minimum and maximum timestamp bounds.
Updates #28358.
Release note: None