Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
121133: sql_test: deflake TestIndexBackfillFractionTracking r=miraradeva a=miraradeva

TestIndexBackfillFractionTracking performs some splits, followed by index backfills. When running under race, the lease of the RHS, installed by the split, can expire and cause the `no valid lease` error.

This patch uses `FindRangeLeaseEx` instead of `FindRangeLeaseHolder` to find the lease; the former doesn't check the validity of the lease; instead it returns a timestamp from a node's clock.

We've seen this before in #9721.

Fixes: #120627

Release note: None

121145: githooks: don't require matching `.git` in `pre-push` hook r=dt,jlinder a=rickystewart

This never triggered for me because my remote doesn't have the `.git` suffix (it's not required). This should help it kick in more.

Epic: none
Part of: DEVINF-1082
Release note: None

Co-authored-by: Mira Radeva <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
  • Loading branch information
3 people committed Mar 26, 2024
3 parents 2cf36f7 + 3527a63 + 7443577 commit 7ff7978
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 11 deletions.
2 changes: 1 addition & 1 deletion githooks/pre-push
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ set -uo pipefail
# deny push of a head but not a tag to cockroachdb/cochroach ssh and http URLs.
while read local_ref local_sha remote_ref remote_sha
do
if [[ "$remote_ref" == "refs/heads/"* ]] && [[ "$2" == *"cockroachdb/cockroach.git"* ]]; then
if [[ "$remote_ref" == "refs/heads/"* ]] && [[ "$2" == *"cockroachdb/cockroach"* ]]; then
echo "Refusing to push to $remote_ref on $2."
echo "Push your branch to your own fork and open a PR from there."
echo "If you just want to see what CI thinks, you can push branch:refs/ci/branch to trigger a CI run."
Expand Down
3 changes: 2 additions & 1 deletion pkg/sql/mvcc_backfiller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -671,10 +671,11 @@ func splitIndex(
return err
}

holder, err := tc.FindRangeLeaseHolder(rangeDesc, nil)
li, _, err := tc.FindRangeLeaseEx(ctx, rangeDesc, nil)
if err != nil {
return err
}
holder := li.CurrentOrProspective().Replica

_, rightRange, err := tc.Server(int(holder.NodeID) - 1).SplitRange(pik)
if err != nil {
Expand Down
15 changes: 15 additions & 0 deletions pkg/testutils/serverutils/test_cluster_shim.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,21 @@ type TestClusterInterface interface {
hint *roachpb.ReplicationTarget,
) (roachpb.ReplicationTarget, error)

// FindRangeLeaseEx returns information about a range's lease. As opposed to
// FindRangeLeaseHolder, it doesn't check the validity of the lease; instead it
// returns a timestamp from a node's clock.
//
// If hint is not nil, the respective node will be queried. If that node doesn't
// have a replica able to serve a LeaseInfoRequest, an error will be returned.
// If hint is nil, the first node is queried. In either case, if the returned
// lease is not valid, it's possible that the returned lease information is
// stale - i.e. there might be a newer lease unbeknownst to the queried node.
FindRangeLeaseEx(
ctx context.Context,
rangeDesc roachpb.RangeDescriptor,
hint *roachpb.ReplicationTarget,
) (_ roachpb.LeaseInfo, now hlc.ClockTimestamp, _ error)

// TransferRangeLease transfers the lease for a range from whoever has it to
// a particular store. That store must already have a replica of the range. If
// that replica already has the (active) lease, this method is a no-op.
Expand Down
10 changes: 1 addition & 9 deletions pkg/testutils/testcluster/testcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -1278,15 +1278,7 @@ func (tc *TestCluster) FindRangeLease(
return l.CurrentOrProspective(), now, err
}

// FindRangeLeaseEx returns information about a range's lease. As opposed to
// FindRangeLeaseHolder, it doesn't check the validity of the lease; instead it
// returns a timestamp from a node's clock.
//
// If hint is not nil, the respective node will be queried. If that node doesn't
// have a replica able to serve a LeaseInfoRequest, an error will be returned.
// If hint is nil, the first node is queried. In either case, if the returned
// lease is not valid, it's possible that the returned lease information is
// stale - i.e. there might be a newer lease unbeknownst to the queried node.
// FindRangeLeaseEx is part of TestClusterInterface.
func (tc *TestCluster) FindRangeLeaseEx(
ctx context.Context, rangeDesc roachpb.RangeDescriptor, hint *roachpb.ReplicationTarget,
) (_ roachpb.LeaseInfo, now hlc.ClockTimestamp, _ error) {
Expand Down

0 comments on commit 7ff7978

Please sign in to comment.