From 7959165f3e56671bb94bd9a961b97d2680ab388b Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 18 Feb 2021 16:38:19 -0500 Subject: [PATCH] kv: add TestStoreRangeSplitAndMergeWithGlobalReads Made possible by #60567. This commit adds a new test called TestStoreRangeSplitAndMergeWithGlobalReads that tests that a range configured to serve global reads can be split and merged. In essence, this tests whether the split and merge transactions can handle having their timestamp bumped by the closed timestamp on the ranges they're operating on. The test revealed that range merges did have issues in these cases, because SubsumeRequests assumed that the intent on the RHS's local descriptor was below the node's HLC clock. This is no longer always true, so we now perform the inconsistent scan at hlc.MaxTimestamp, just like QueryIntent requests do. --- pkg/kv/kvserver/batcheval/cmd_subsume.go | 5 ++- pkg/kv/kvserver/client_split_test.go | 57 ++++++++++++++++++++++++ pkg/kv/kvserver/replica.go | 8 ++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/pkg/kv/kvserver/batcheval/cmd_subsume.go b/pkg/kv/kvserver/batcheval/cmd_subsume.go index 77797e62b7c4..28e9f1c43603 100644 --- a/pkg/kv/kvserver/batcheval/cmd_subsume.go +++ b/pkg/kv/kvserver/batcheval/cmd_subsume.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" + "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/errors" ) @@ -92,14 +93,14 @@ func Subsume( // Sanity check the caller has initiated a merge transaction by checking for // a deletion intent on the local range descriptor. descKey := keys.RangeDescriptorKey(desc.StartKey) - _, intent, err := storage.MVCCGet(ctx, readWriter, descKey, cArgs.Header.Timestamp, + _, intent, err := storage.MVCCGet(ctx, readWriter, descKey, hlc.MaxTimestamp, storage.MVCCGetOptions{Inconsistent: true}) if err != nil { return result.Result{}, errors.Errorf("fetching local range descriptor: %s", err) } else if intent == nil { return result.Result{}, errors.AssertionFailedf("range missing intent on its local descriptor") } - val, _, err := storage.MVCCGetAsTxn(ctx, readWriter, descKey, cArgs.Header.Timestamp, intent.Txn) + val, _, err := storage.MVCCGetAsTxn(ctx, readWriter, descKey, intent.Txn.WriteTimestamp, intent.Txn) if err != nil { return result.Result{}, errors.Errorf("fetching local range descriptor as txn: %s", err) } else if val != nil { diff --git a/pkg/kv/kvserver/client_split_test.go b/pkg/kv/kvserver/client_split_test.go index dca3120999b8..3cff9b9eef7a 100644 --- a/pkg/kv/kvserver/client_split_test.go +++ b/pkg/kv/kvserver/client_split_test.go @@ -3545,3 +3545,60 @@ func TestSplitBlocksReadsToRHS(t *testing.T) { } require.Nil(t, g.Wait()) } + +// TestStoreRangeSplitAndMergeWithGlobalReads tests that a range configured to +// serve global reads can be split and merged. In essence, this tests whether +// the split and merge transactions can handle having their timestamp bumped by +// the closed timestamp on the ranges they're operating on. +func TestStoreRangeSplitAndMergeWithGlobalReads(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + serv, _, _ := serverutils.StartServer(t, base.TestServerArgs{ + Knobs: base.TestingKnobs{ + Store: &kvserver.StoreTestingKnobs{ + DisableMergeQueue: true, + }, + }, + }) + s := serv.(*server.TestServer) + defer s.Stopper().Stop(ctx) + store, err := s.Stores().GetStore(s.GetFirstStoreID()) + require.NoError(t, err) + config.TestingSetupZoneConfigHook(s.Stopper()) + + // Set global reads. + descID := uint32(keys.MinUserDescID) + descKey := keys.SystemSQLCodec.TablePrefix(descID) + zoneConfig := zonepb.DefaultZoneConfig() + zoneConfig.GlobalReads = proto.Bool(true) + config.TestingSetZoneConfig(config.SystemTenantObjectID(descID), zoneConfig) + + // Trigger gossip callback and wait for propagation + require.NoError(t, store.Gossip().AddInfoProto(gossip.KeySystemConfig, &config.SystemConfigEntries{}, 0)) + testutils.SucceedsSoon(t, func() error { + repl := store.LookupReplica(roachpb.RKey(descKey)) + if repl.ClosedTimestampPolicy() != roachpb.LEAD_FOR_GLOBAL_READS { + return errors.Errorf("expected LEAD_FOR_GLOBAL_READS policy") + } + return nil + }) + + // Split the range. Should succeed. + splitKey := append(descKey, []byte("split")...) + splitArgs := adminSplitArgs(splitKey) + _, pErr := kv.SendWrapped(ctx, store.TestSender(), splitArgs) + require.Nil(t, pErr) + + repl := store.LookupReplica(roachpb.RKey(splitKey)) + require.Equal(t, splitKey, repl.Desc().StartKey.AsRawKey()) + + // Merge the range. Should succeed. + mergeArgs := adminMergeArgs(descKey) + _, pErr = kv.SendWrapped(ctx, store.TestSender(), mergeArgs) + require.Nil(t, pErr) + + repl = store.LookupReplica(roachpb.RKey(splitKey)) + require.Equal(t, descKey, repl.Desc().StartKey.AsRawKey()) +} diff --git a/pkg/kv/kvserver/replica.go b/pkg/kv/kvserver/replica.go index 8014cb82bfeb..dadbcf6f455e 100644 --- a/pkg/kv/kvserver/replica.go +++ b/pkg/kv/kvserver/replica.go @@ -722,6 +722,14 @@ func (r *Replica) descRLocked() *roachpb.RangeDescriptor { return r.mu.state.Desc } +// ClosedTimestampPolicy returns the closed timestamp policy of the range, which +// is updated asynchronously through gossip of zone configurations. +func (r *Replica) ClosedTimestampPolicy() roachpb.RangeClosedTimestampPolicy { + r.mu.RLock() + defer r.mu.RUnlock() + return r.closedTimestampPolicyRLocked() +} + func (r *Replica) closedTimestampPolicyRLocked() roachpb.RangeClosedTimestampPolicy { if r.mu.zone.GlobalReads != nil && *r.mu.zone.GlobalReads { return roachpb.LEAD_FOR_GLOBAL_READS