Merge #152294

152294: storage,rditer: move shared storage CCL implementation bits r=xinhaoz a=jbowens

Ever since the license change, there's been no need for the indirection of defining this code in the ccl package.

Co-authored-by: Jackson Owens <[email protected]>

Author: craig[bot]

pkg/ccl/storageccl/engineccl/BUILD.bazel

@@ -6,28 +6,18 @@ go_library(
         "ctr_stream.go",
         "encrypted_fs.go",
         "pebble_key_manager.go",
-        "shared_storage.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/storageccl/engineccl",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/ccl/utilccl",
-        "//pkg/kv/kvserver/rditer",
-        "//pkg/roachpb",
-        "//pkg/settings/cluster",
-        "//pkg/storage",
         "//pkg/storage/enginepb",
         "//pkg/storage/fs",
         "//pkg/storage/storageconfig",
         "//pkg/util/log",
         "//pkg/util/protoutil",
         "//pkg/util/syncutil",
-        "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//oserror",
-        "@com_github_cockroachdb_pebble//:pebble",
-        "@com_github_cockroachdb_pebble//objstorage/remote",
-        "@com_github_cockroachdb_pebble//rangekey",
         "@com_github_cockroachdb_pebble//vfs",
         "@com_github_cockroachdb_pebble//vfs/atomicfs",
         "@com_github_gogo_protobuf//proto",

pkg/ccl/storageccl/engineccl/shared_storage.go

@@ -358,12 +358,6 @@ func (kvSS *kvBatchSnapshotStrategy) Send(
 		return nil
 	}
 
-	// If snapshots containing shared files are allowed, and this range is a
-	// non-system range, take advantage of shared storage to minimize the amount
-	// of data we're iterating on and sending over the network.
-	sharedReplicate := header.SharedReplicate && rditer.IterateReplicaKeySpansShared != nil
-	externalReplicate := header.ExternalReplicate && rditer.IterateReplicaKeySpansShared != nil
-
 	iterateRKSpansVisitor := func(iter storage.EngineIterator, _ roachpb.Span) error {
 		timingTag.start("iter")
 		defer timingTag.stop("iter")
@@ -419,7 +413,7 @@ func (kvSS *kvBatchSnapshotStrategy) Send(
 			LockTable:  true,
 			// In shared/external mode, the user span come from external SSTs and
 			// are not iterated over here.
-			UserKeys: !(sharedReplicate || externalReplicate),
+			UserKeys: !(header.SharedReplicate || header.ExternalReplicate),
 		},
 		ReplicatedByRangeID:   true,
 		UnreplicatedByRangeID: false,
@@ -428,9 +422,12 @@ func (kvSS *kvBatchSnapshotStrategy) Send(
 	}
 
 	var valBuf []byte
-	if sharedReplicate || externalReplicate {
+	// If snapshots containing shared files are allowed, and this range is a
+	// non-system range, take advantage of shared storage to minimize the amount
+	// of data we're iterating on and sending over the network.
+	if header.SharedReplicate || header.ExternalReplicate {
 		var sharedVisitor func(sst *pebble.SharedSSTMeta) error
-		if sharedReplicate {
+		if header.SharedReplicate {
 			sharedVisitor = func(sst *pebble.SharedSSTMeta) error {
 				sharedSSTCount++
 				snap.sharedBackings = append(snap.sharedBackings, sst.Backing)
@@ -459,7 +456,7 @@ func (kvSS *kvBatchSnapshotStrategy) Send(
 			}
 		}
 		var externalVisitor func(sst *pebble.ExternalFile) error
-		if externalReplicate {
+		if header.ExternalReplicate {
 			externalVisitor = func(sst *pebble.ExternalFile) error {
 				externalSSTCount++
 				externalSSTs = append(externalSSTs, kvserverpb.SnapshotRequest_ExternalTable{

pkg/kv/kvserver/kv_snapshot_strategy.go

@@ -454,23 +454,45 @@ func IterateReplicaKeySpans(
 }
 
 // IterateReplicaKeySpansShared is a shared-replicate version of
-// IterateReplicaKeySpans. See definitions of this method for how it is
-// implemented.
+// IterateReplicaKeySpans. IterateReplicaKeySpansShared iterates over the
+// range's user key span, skipping any keys present in shared files. It calls
+// the appropriate visitor function for the type of key visited, namely, point
+// keys, range deletes and range keys. Shared files that are skipped during this
+// iteration are also surfaced through a dedicated visitor. Note that this
+// method only iterates over a range's user key span; IterateReplicaKeySpans
+// must be called to iterate over the other key spans.
 //
-// The impl of this method along with a comment is in
-// engineccl/shared_storage.go.
-var IterateReplicaKeySpansShared func(
+// If this method returns pebble.ErrInvalidSkipSharedIteration, only the shared
+// external visitors may have been invoked. In particular, no local data has
+// been visited yet.  The above contract appears true for the current
+// implementation of this method, but is likely untested.
+//
+// Must use a reader with consistent iterators.
+func IterateReplicaKeySpansShared(
 	ctx context.Context,
 	desc *roachpb.RangeDescriptor,
 	st *cluster.Settings,
-	clusterID uuid.UUID,
+	_ uuid.UUID,
 	reader storage.Reader,
 	visitPoint func(key *pebble.InternalKey, val pebble.LazyValue, info pebble.IteratorLevel) error,
 	visitRangeDel func(start, end []byte, seqNum pebble.SeqNum) error,
 	visitRangeKey func(start, end []byte, keys []rangekey.Key) error,
 	visitSharedFile func(sst *pebble.SharedSSTMeta) error,
 	visitExternalFile func(sst *pebble.ExternalFile) error,
-) error
+) error {
+	if !reader.ConsistentIterators() {
+		panic("reader must provide consistent iterators")
+	}
+	spans := Select(desc.RangeID, SelectOpts{
+		Ranged: SelectRangedOptions{
+			RSpan:    desc.RSpan(),
+			UserKeys: true,
+		},
+	})
+	span := spans[0]
+	return reader.ScanInternal(ctx, span.Key, span.EndKey, visitPoint, visitRangeDel,
+		visitRangeKey, visitSharedFile, visitExternalFile)
+}
 
 // IterateOptions instructs how points and ranges should be presented to visitor
 // and if iterators should be visited in forward or reverse order.

pkg/kv/kvserver/rditer/replica_data_iter.go

@@ -848,10 +848,6 @@ func (r remoteStorageAdaptor) CreateStorage(locator remote.Locator) (remote.Stor
 	return &externalStorageWrapper{p: r.p, ctx: r.ctx, es: es}, err
 }
 
-// ConfigureForSharedStorage is used to configure a pebble Options for shared
-// storage.
-var ConfigureForSharedStorage func(opts *pebble.Options, storage remote.Storage) error
-
 // newPebble creates a new Pebble instance, at the specified path.
 // Do not use directly (except in test); use Open instead.
 //
@@ -1064,9 +1060,6 @@ func newPebble(ctx context.Context, cfg engineConfig) (p *Pebble, err error) {
 	// in it is needed for CRDB to function properly.
 	if cfg.sharedStorage != nil {
 		esWrapper := &externalStorageWrapper{p: p, es: cfg.sharedStorage, ctx: logCtx}
-		if ConfigureForSharedStorage == nil {
-			return nil, errors.New("shared storage requires CCL features")
-		}
 		if err := ConfigureForSharedStorage(cfg.opts, esWrapper); err != nil {
 			return nil, errors.Wrap(err, "error when configuring shared storage")
 		}

pkg/storage/pebble.go

@@ -12,9 +12,21 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/cloud"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/pebble"
 	"github.com/cockroachdb/pebble/objstorage/remote"
 )
 
+// ConfigureForSharedStorage mutates the provided Pebble options to use shared
+// storage using the provided remote storage.
+func ConfigureForSharedStorage(opts *pebble.Options, remoteStorage remote.Storage) error {
+	opts.Experimental.RemoteStorage = remote.MakeSimpleFactory(map[remote.Locator]remote.Storage{
+		"": remoteStorage,
+	})
+	opts.Experimental.CreateOnShared = remote.CreateOnSharedLower
+	opts.Experimental.CreateOnSharedLocator = ""
+	return nil
+}
+
 // externalStorageReader implements remote.ObjectReader on top of
 // cloud.ExternalStorage..
 type externalStorageReader struct {