storage: Wraps the ExternalStorage factory methods in an ExternalStorageBuilder

[adityamaru]

Previously, the ExternalStorage factory methods were initialized on
creation of a NewServer(). These closures were then plumbed through
various componenets such as sqlServer, storeConfig etc. The closures
were invoked whenever an ExternalStorage object needed creation.

This refactor maintains the semantics of the ExternalStorage factory
methods, but introduces an ExternalStorageBuilder. This object
implements the factory methods and stores relevant configuration
parameters. The main goal of this refactor was to separate the
"creation" and "initialization" stages of the ExternalStorageBuilder.
While the object is created and plumbed on creation of a NewServer(),
certain config params (eg: storage.Engine) are only initialized on
server.Start(). The plumbing of an object pointer as compared to
closures allows us to cleanly configure the builder in server.Start()
and propagate this information for future use in the factory methods.

Using and plumbing a dedicated builder also seems like a design
improvement to bookkeeping closure variables. This lays the groundwork
for the user scoped storage which is being discussed in #47211.

Release note: None

[cockroach-teamcity]

This change is 

[adityamaru]

@miretskiy the use of engine.Storage in ExternalStorageBuilder creates a forbidden chain:

build.go:128: ./pkg/sql depends on: ./pkg/sql/colexec depends on: ./pkg/sql/colexecbase depends on: ./pkg/sql/execinfra depends on: ./pkg/storage/cloud imports ./pkg/storage, which is forbidden

For the purpose of this refactor I have used opaque interfaces to get CI passing. In an immediate follow-up PR (as discussed offline with @dt) I will add a BuilderInterface to pkg/storage/cloud and split the concrete implementation of the builder into a separate pkg/storage/cloudfactory (name TBD). pkg/server which creates/initializes the builder can depend on pkg/storage/cloudfactory (and in turn use engine.Storage), but pkg/sql and everyone else will only depend on the interface in pkg/storage/cloud, thereby breaking the chain.

[miretskiy]

Reviewed 2 of 44 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, and @miretskiy)

---

pkg/base/config.go, line 728 at r1 (raw file):

// ExternalStorageConfig describes various configuration options pertaining
// to external storage implementations.
type ExternalStorageConfig struct {

Should this renaming be pulled into separate PR?

---

pkg/server/server.go, line 402 at r1 (raw file):

			),
		)
	}

My thinking around this refactoring was to actually try to keep these functors for the purpose of initial refactoring. something like:

externalStorageBuilder := cloud.NewExternalStorageBuilder()

...
ExternalStorageBuilder: externalStorageBuilder
ExternalStorageFromURI := func(...) (cloud.ExternalStorage, error) {
return externalStorageBuilder.NewStorageFromUri(...);
}
ExternalStorage := func(...) (cloud.ExternalStorage, error) {
return externalStorageBuilder.NewStorage(...);
}

My thinking was that at the very least this would allow you to keep this refactoring focused, basically just to this file (at least initially), without having to touch 44 other places, while allowing you to add logic to later init externalStorageBuilder in the Start()

Furthermore, I would argue that keeping the builder definition right inside this file is probably okay too (as to avoid circular dependencies issues).

What do you think?

---

pkg/storage/cloud/external_storage.go, line 284 at r1 (raw file):

	// Factory methods used by the builder to create external storage objects.
	ExternalStorageFactoryImpl        ExternalStorageFactory
	ExternalStorageFromURIFactoryImpl ExternalStorageFromURIFactory

I am a bit confused about the need to have FactoryImpl inside builder....

---

pkg/storage/cloud/external_storage.go, line 289 at r1 (raw file):

// NewExternalStorageBuilder returns a new ExternalStorageBuilder object. Init
// must be called before its factory methods can be used.
func NewExternalStorageBuilder() ExternalStorageBuilder {

I'd say anything called NewXXX() should return a pointer.

[adityamaru]

Discussed offline with @miretskiy, we are going to approach this refactor in a more phased manner while working towards the same goal. Please hold off on any reviews!