Skip to content

Commit

Permalink
cluster: remove VersionPushTxnToInclusive and PushTxnReq.DeprecatedNow
Browse files Browse the repository at this point in the history
I had it commented that we could also remove `InclusivePushTo` in 19.2,
but I now think that was incorrect. We still need to set it to true to
prevent 19.1 nodes that have yet to hear about the `VersionPushTxnToInclusive`
becoming active from thinking that the Replica sending the PushTxnRequest
is a 2.1 node (which sent exclusive push to timestamps).

Release note: None
  • Loading branch information
nvanbenschoten committed Jun 26, 2019
1 parent 43869ae commit caebf7e
Show file tree
Hide file tree
Showing 8 changed files with 931 additions and 1,022 deletions.
1,882 changes: 915 additions & 967 deletions pkg/roachpb/api.pb.go

Large diffs are not rendered by default.

11 changes: 2 additions & 9 deletions pkg/roachpb/api.proto
Original file line number Diff line number Diff line change
Expand Up @@ -873,15 +873,8 @@ message PushTxnRequest {
// is the timestamp they want the transaction to be pushed to, instead of
// the timestamp before the one they want the transaction to be pushed to.
// It is used to assist that field's migration.
// TODO(nvanbenschoten): Remove this field in 19.2.
// TODO(nvanbenschoten): Remove this field in 19.3.
bool inclusive_push_to = 9;
// DeprecatedNow holds the timestamp used to compare the last heartbeat of the
// pushee against.
//
// The field remains for compatibility with 2.1 nodes. Users should set the
// same value for this field and the batch header timestamp.
// TODO(nvanbenschoten): Remove this field in 19.2.
util.hlc.Timestamp deprecated_now = 5 [(gogoproto.nullable) = false];
// Readers set this to PUSH_TIMESTAMP to move pushee_txn's provisional
// commit timestamp forward. Writers set this to PUSH_ABORT to request
// that pushee_txn be aborted if possible. Inconsistent readers set
Expand All @@ -892,7 +885,7 @@ message PushTxnRequest {
// in PushTxn to either abort or push the timestamp.
bool force = 7;

reserved 8;
reserved 5, 8;
}

// A PushTxnResponse is the return value from the PushTxn() method. It
Expand Down
12 changes: 6 additions & 6 deletions pkg/settings/cluster/cockroach_versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const (
Version2_1
VersionUnreplicatedRaftTruncatedState // see versionsSingleton for details
VersionSideloadedStorageNoReplicaID // see versionsSingleton for details
VersionPushTxnToInclusive
VersionSnapshotsWithoutLog
Version19_1
VersionStart19_2
Expand Down Expand Up @@ -442,11 +441,12 @@ var versionsSingleton = keyedVersions([]keyedVersion{
Key: VersionSideloadedStorageNoReplicaID,
Version: roachpb.Version{Major: 2, Minor: 1, Unstable: 9},
},
{
// VersionPushTxnToInclusive is https://github.com/cockroachdb/cockroach/pull/35297.
Key: VersionPushTxnToInclusive,
Version: roachpb.Version{Major: 2, Minor: 1, Unstable: 10},
},
// Removed.
// {
// // VersionPushTxnToInclusive is https://github.com/cockroachdb/cockroach/pull/35297.
// Key: VersionPushTxnToInclusive,
// Version: roachpb.Version{Major: 2, Minor: 1, Unstable: 10},
// },
{
// VersionSnapshotsWithoutLog is https://github.com/cockroachdb/cockroach/pull/36714.
Key: VersionSnapshotsWithoutLog,
Expand Down
17 changes: 8 additions & 9 deletions pkg/settings/cluster/versionkey_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion pkg/storage/intentresolver/contention_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,6 @@ func (cq *contentionQueue) add(
PusheeTxn: *pusheeTxn,
PushTo: h.Timestamp.Next(),
InclusivePushTo: true,
DeprecatedNow: b.Header.Timestamp,
PushType: roachpb.PUSH_ABORT,
})
log.VEventf(ctx, 3, "%s pushing %s to detect dependency cycles", txnID(curPusher.txn), pusheeTxn.ID.Short())
Expand Down
2 changes: 0 additions & 2 deletions pkg/storage/intentresolver/intent_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,6 @@ func (ir *IntentResolver) MaybePushTransactions(
PusheeTxn: pushTxn,
PushTo: h.Timestamp.Next(),
InclusivePushTo: true,
DeprecatedNow: b.Header.Timestamp,
PushType: pushType,
})
}
Expand Down Expand Up @@ -693,7 +692,6 @@ func (ir *IntentResolver) CleanupTxnIntentsOnGCAsync(
TxnMeta: enginepb.TxnMeta{Priority: enginepb.MaxTxnPriority},
},
PusheeTxn: txn.TxnMeta,
DeprecatedNow: b.Header.Timestamp,
PushType: roachpb.PUSH_ABORT,
InclusivePushTo: true,
})
Expand Down
1 change: 0 additions & 1 deletion pkg/storage/replica.go
Original file line number Diff line number Diff line change
Expand Up @@ -1453,7 +1453,6 @@ func (r *Replica) maybeWatchForMerge(ctx context.Context) error {
TxnMeta: enginepb.TxnMeta{Priority: enginepb.MinTxnPriority},
},
PusheeTxn: intent.Txn,
DeprecatedNow: b.Header.Timestamp,
PushType: roachpb.PUSH_ABORT,
InclusivePushTo: true,
})
Expand Down
27 changes: 0 additions & 27 deletions pkg/storage/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -2804,32 +2804,6 @@ func (s *Store) Send(
return nil, roachpb.NewError(err)
}

// In 2.1 it was possible for nodes to send PushTxn requests without
// properly reflecting the time that they wanted the push to happen
// in the batch's header timestamp. Ensure that this timestamp is
// sufficiently advanced to prevent lost timestamp cache updates.
// TODO(nvanbenschoten): Remove this all in 19.2.
if !s.cfg.Settings.Version.IsActive(cluster.VersionPushTxnToInclusive) {
for _, union := range ba.Requests {
if pushReq, ok := union.GetInner().(*roachpb.PushTxnRequest); ok {
ba.Timestamp.Forward(pushReq.DeprecatedNow)

// While here, correct the request's PushTo arg. Before
// VersionPushTxnToInclusive, pushers would provide _their_
// timestamp instead of one logical tick past their timestamp
// for the PushTo arg. Handle this by bumping the PushTo arg.
// We only do this if the InclusivePushTo arg is false, which
// allows us to distinguish between 2.1 nodes and 19.1. nodes.
// Since we only observe this field when the cluster version
// is not active, we don't need to send it or observe it in
// 19.2.
if !pushReq.InclusivePushTo {
pushReq.PushTo = pushReq.PushTo.Next()
}
}
}
}

if s.cfg.TestingKnobs.ClockBeforeSend != nil {
s.cfg.TestingKnobs.ClockBeforeSend(s.cfg.Clock, ba)
}
Expand Down Expand Up @@ -3163,7 +3137,6 @@ func (s *Store) maybeWaitForPushee(
// to push a txn which has expired.
now := s.Clock().Now()
ba.Timestamp.Forward(now)
pushReqCopy.DeprecatedNow.Forward(now)
ba.Requests = nil
ba.Add(&pushReqCopy)
} else if ba.IsSingleQueryTxnRequest() {
Expand Down

0 comments on commit caebf7e

Please sign in to comment.