Skip to content

Commit

Permalink
Merge #26939 #26972
Browse files Browse the repository at this point in the history 
26939: roachpb,storage: remove DeprecatedVerifyChecksum request r=nvanbenschoten,tschottdorf a=benesch

DeprecatedVerifyChecksum request was removed prior to September 2016. It
is not sent by a CockroachDB 2.0 cluster, so it's safe to remove in 2.1.

---

Someone more familiar with prop-eval KV should double-check that this is actually safe.

26972: sem/tree: remove TestClusterTimestampConversion r=andreimatei a=andreimatei

I can't really tell what this test is testing exactly, but what it does
is really weird - it creates a TestingEvalCcontext and then it peeks
inside it to update the txn proto's timestamp, only to the retrieve that
timestamp and assert some serialization on it.
This is bizarre, and also changing a txn's proto like that is not going
to be a thing any more - I'm making this proto not be exposed any more,
and definitely not writable by the world. In fact, it never was intended
to be modified by clients.

If a test really needs a transaction at a particular timestamp and the
test needs to operate at a high level, there's txn.SetFixedTimestamp().
But that requires a txn to actually exist; the mocking done by
tree.NewTestingEvalContext() is not sufficient.
I'd like to remove this test...

Release note: None

Co-authored-by: Nikhil Benesch <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
  • Loading branch information
@benesch @andreimatei
3 people committed Jun 26, 2018
3 parents 3676a3f + 0e94a78 + f756cb3 commit 95ed31b
Show file tree
Hide file tree
Showing 10 changed files with 802 additions and 1,300 deletions.
26 changes: 8 additions & 18 deletions pkg/roachpb/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -568,9 +568,6 @@ func (*LeaseInfoRequest) Method() Method { return LeaseInfo }
// Method implements the Request interface.
func (*ComputeChecksumRequest) Method() Method { return ComputeChecksum }

// Method implements the Request interface.
func (*DeprecatedVerifyChecksumRequest) Method() Method { return DeprecatedVerifyChecksum }

// Method implements the Request interface.
func (*WriteBatchRequest) Method() Method { return WriteBatch }

Expand Down Expand Up @@ -781,12 +778,6 @@ func (ccr *ComputeChecksumRequest) ShallowCopy() Request {
return &shallowCopy
}

// ShallowCopy implements the Request interface.
func (dvcr *DeprecatedVerifyChecksumRequest) ShallowCopy() Request {
shallowCopy := *dvcr
return &shallowCopy
}

// ShallowCopy implements the Request interface.
func (r *WriteBatchRequest) ShallowCopy() Request {
shallowCopy := *r
Expand Down Expand Up @@ -1081,15 +1072,14 @@ func (*TransferLeaseRequest) flags() int {
// lease holder.
return isWrite | isAlone | skipLeaseCheck
}
func (*RecomputeStatsRequest) flags() int { return isWrite | isAlone }
func (*ComputeChecksumRequest) flags() int { return isWrite | isRange }
func (*DeprecatedVerifyChecksumRequest) flags() int { return isWrite }
func (*CheckConsistencyRequest) flags() int { return isAdmin | isRange }
func (*WriteBatchRequest) flags() int { return isWrite | isRange }
func (*ExportRequest) flags() int { return isRead | isRange | updatesReadTSCache }
func (*ImportRequest) flags() int { return isAdmin | isAlone }
func (*AdminScatterRequest) flags() int { return isAdmin | isAlone | isRange }
func (*AddSSTableRequest) flags() int { return isWrite | isAlone | isRange }
func (*RecomputeStatsRequest) flags() int { return isWrite | isAlone }
func (*ComputeChecksumRequest) flags() int { return isWrite | isRange }
func (*CheckConsistencyRequest) flags() int { return isAdmin | isRange }
func (*WriteBatchRequest) flags() int { return isWrite | isRange }
func (*ExportRequest) flags() int { return isRead | isRange | updatesReadTSCache }
func (*ImportRequest) flags() int { return isAdmin | isAlone }
func (*AdminScatterRequest) flags() int { return isAdmin | isAlone | isRange }
func (*AddSSTableRequest) flags() int { return isWrite | isAlone | isRange }

// RefreshRequest and RefreshRangeRequest both list
// updates(Read)TSCache, though they actually update the read or write
Expand Down
1,764 changes: 681 additions & 1,083 deletions pkg/roachpb/api.pb.go
View file

Large diffs are not rendered by default.

24 changes: 2 additions & 22 deletions pkg/roachpb/api.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,16 +971,6 @@ message ComputeChecksumResponse {
ResponseHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
}

message DeprecatedVerifyChecksumRequest {
option (gogoproto.equal) = true;

RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
}

message DeprecatedVerifyChecksumResponse {
ResponseHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
}

enum ExportStorageProvider {
Unknown = 0;
LocalFile = 1;
Expand Down Expand Up @@ -1231,11 +1221,6 @@ message RefreshRangeResponse {
// A RequestUnion contains exactly one of the requests.
// The values added here must match those in ResponseUnion.
//
// WARNING: Do not remove fields from RequestUnion. Instead, remove
// all non-header fields from the request message and prepend its
// name with "Deprecated". See DeprecatedVerifyChecksumRequest for an
// example.
//
// Be cautious about deprecating fields as doing so can lead to inconsistencies
// between replicas.
message RequestUnion {
Expand Down Expand Up @@ -1266,7 +1251,7 @@ message RequestUnion {
RequestLeaseRequest request_lease = 20;
ReverseScanRequest reverse_scan = 21;
ComputeChecksumRequest compute_checksum = 22;
DeprecatedVerifyChecksumRequest deprecated_verify_checksum = 23;
reserved 23;
CheckConsistencyRequest check_consistency = 24;
NoopRequest noop = 25;
InitPutRequest init_put = 26;
Expand All @@ -1287,11 +1272,6 @@ message RequestUnion {

// A ResponseUnion contains exactly one of the responses.
// The values added here must match those in RequestUnion.
//
// WARNING: Do not remove fields from ResponseUnion. Instead, remove
// all non-header fields from the response message and prepend its
// name with "Deprecated". See DeprecatedVerifyChecksumResponse for an
// example.
message ResponseUnion {
option (gogoproto.onlyone) = true;

Expand Down Expand Up @@ -1320,7 +1300,7 @@ message ResponseUnion {
RequestLeaseResponse request_lease = 20;
ReverseScanResponse reverse_scan = 21;
ComputeChecksumResponse compute_checksum = 22;
DeprecatedVerifyChecksumResponse deprecated_verify_checksum = 23;
reserved 23;
CheckConsistencyResponse check_consistency = 24;
NoopResponse noop = 25;
InitPutResponse init_put = 26;
Expand Down
33 changes: 0 additions & 33 deletions pkg/roachpb/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package roachpb

import (
"encoding/hex"
"reflect"
"testing"
)
Expand Down Expand Up @@ -113,35 +112,3 @@ func TestMustSetInner(t *testing.T) {
t.Fatalf("unexpected response union: %+v", res)
}
}

func TestDeprecatedVerifyChecksumRequest(t *testing.T) {
t.Skip("TODO(nvanbenschoten): fix")
// hexData was generated using the following code snippet. The batch contains
// a VerifyChecksumRequest which is no longer part of RequestUnion.
//
// var ba BatchRequest
// ba.Add(&VerifyChecksumRequest{})
// var v Value
// if err := v.SetProto(&ba); err != nil {
// t.Fatal(err)
// }
// fmt.Printf("%s\n", hex.EncodeToString(v.RawBytes))

hexData := `00000000030a1f0a0408001000120608001000180018002100000000000000003000400048001219ba01160a0010001a1000000000000000000000000000000000`
data, err := hex.DecodeString(hexData)
if err != nil {
t.Fatal(err)
}
v := Value{RawBytes: data}
var ba BatchRequest
if err := v.GetProto(&ba); err != nil {
t.Fatal(err)
}
// This previously failed with a nil-pointer conversion error in
// BatchRequest.GetArg() because of the removal of
// RequestUnion.VerifyChecksum. We've now re-added that member as
// RequestUnion.DeprecatedVerifyChecksum.
if ba.IsLeaseRequest() {
t.Fatal("unexpected success")
}
}
Loading

0 comments on commit 95ed31b

Please sign in to comment.