laskoviymishka commented on code in PR #2291:
URL: https://github.com/apache/iceberg-go/pull/2291#discussion_r4368745449
##########
io/registry.go:
##########
@@ -28,53 +29,121 @@ import (
"github.com/apache/iceberg-go/internal/schemes"
)
-type registry map[string]SchemeFactory
+// SchemeFactory creates an IO implementation for a URI and its properties.
+type SchemeFactory func(ctx context.Context, parsed *url.URL, props
map[string]string) (IO, error)
var (
- regMutex sync.RWMutex
- defaultRegistry = registry{}
+ // ErrSchemeAlreadyRegistered indicates that a registry already has a
factory for a scheme.
+ ErrSchemeAlreadyRegistered = errors.New("io scheme already registered")
+ // ErrInvalidSchemeFactory indicates that an attempt was made to
register a nil factory.
+ ErrInvalidSchemeFactory = errors.New("io scheme factory is nil")
)
-// SchemeFactory is a function that creates an IO implementation for a given
URI and properties.
-type SchemeFactory func(ctx context.Context, parsed *url.URL, props
map[string]string) (IO, error)
+// Registry manages FileIO factories for URI schemes. A registry may be used by
+// multiple goroutines. Factories run without the registry lock held, so they
+// may register other schemes or load additional FileIO instances.
+type Registry struct {
+ mu sync.RWMutex
+ factories map[string]SchemeFactory
+}
+
+func builtInRegistry() *Registry {
+ localFSFactory := func(context.Context, *url.URL, map[string]string)
(IO, error) {
+ return LocalFS{}, nil
+ }
+
+ return &Registry{factories: map[string]SchemeFactory{
+ "": localFSFactory,
+ "file": localFSFactory,
+ }}
+}
+
+// defaultRegistry is populated by side-effect imports of the cloud providers.
+// Independent registries clone these registrations rather than sharing the
map.
+var defaultRegistry = builtInRegistry()
-// Register adds a new scheme factory to the registry. If the scheme is
already registered, it will panic.
-func Register(scheme string, factory SchemeFactory) {
+// NewRegistry creates an independent snapshot of the globally registered
+// schemes, including cloud providers already registered through side-effect
+// imports. Future registrations in either registry do not affect the other.
Review Comment:
Should the doc warn that a package-level `NewRegistry()` var can miss
backends? In a repro binary that blank-imports `io/gocloud/s3`, two packages
declaring `var Reg = io.NewRegistry()` and importing only `io` both got `[ file
mem]`, while the global registry had `s3`.
```suggestion
// imports. Future registrations in either registry do not affect the other.
// Called from a package-level var initializer, it can miss backends that the
// calling package does not import.
```
##########
website/src/configuration.md:
##########
@@ -306,17 +306,29 @@ import (
)
func init() {
- io.Register("myfs", func(ctx context.Context, parsed *url.URL, props
map[string]string) (io.IO, error) {
+ io.MustRegister("myfs", func(ctx context.Context, parsed *url.URL, props
map[string]string) (io.IO, error) {
return newMyFS(parsed, props)
})
}
```
-`io.Register` panics on `nil` factory or duplicate scheme. Built-in schemes:
`file`, `""` (the empty scheme).
+`io.MustRegister` panics for invalid or duplicate schemes, so it is suitable
for provider `init` functions. `io.Register` now returns
`ErrInvalidSchemeFactory` or `ErrSchemeAlreadyRegistered` instead of panicking.
Built-in schemes: `file`, `""` (the empty scheme).
Cloud schemes (`s3`, `gs`, `abfs`, etc.) are registered only when the matching
backend package under `io/gocloud` is blank-imported.
`io.GetRegisteredSchemes()` returns the current scheme list;
`io.Unregister(scheme)` removes one.
+To register schemes at runtime without changing other users of the same
process, create an independent registry:
Review Comment:
Can this line say that catalogs and `view.CreateView` still resolve through
the default registry? All 20 places in catalog/ and view/ that build a FileIO
call the global `LoadFS`/`LoadFSFunc`, the same 20 as on `main` (for example
[`view.CreateView`](https://github.com/apache/iceberg-go/blob/34a72c46fe8797aa824091713266fc9cad014a25/view/view.go#L219)).
```suggestion
To register schemes at runtime without changing other users of the same
process, create an independent registry (catalogs and `view.CreateView` still
resolve through the default registry):
```
##########
io/registry.go:
##########
@@ -28,53 +29,121 @@ import (
"github.com/apache/iceberg-go/internal/schemes"
)
-type registry map[string]SchemeFactory
+// SchemeFactory creates an IO implementation for a URI and its properties.
+type SchemeFactory func(ctx context.Context, parsed *url.URL, props
map[string]string) (IO, error)
var (
- regMutex sync.RWMutex
- defaultRegistry = registry{}
+ // ErrSchemeAlreadyRegistered indicates that a registry already has a
factory for a scheme.
+ ErrSchemeAlreadyRegistered = errors.New("io scheme already registered")
+ // ErrInvalidSchemeFactory indicates that an attempt was made to
register a nil factory.
+ ErrInvalidSchemeFactory = errors.New("io scheme factory is nil")
)
-// SchemeFactory is a function that creates an IO implementation for a given
URI and properties.
-type SchemeFactory func(ctx context.Context, parsed *url.URL, props
map[string]string) (IO, error)
+// Registry manages FileIO factories for URI schemes. A registry may be used by
+// multiple goroutines. Factories run without the registry lock held, so they
+// may register other schemes or load additional FileIO instances.
+type Registry struct {
+ mu sync.RWMutex
+ factories map[string]SchemeFactory
+}
+
+func builtInRegistry() *Registry {
+ localFSFactory := func(context.Context, *url.URL, map[string]string)
(IO, error) {
+ return LocalFS{}, nil
+ }
+
+ return &Registry{factories: map[string]SchemeFactory{
+ "": localFSFactory,
+ "file": localFSFactory,
+ }}
+}
+
+// defaultRegistry is populated by side-effect imports of the cloud providers.
+// Independent registries clone these registrations rather than sharing the
map.
+var defaultRegistry = builtInRegistry()
-// Register adds a new scheme factory to the registry. If the scheme is
already registered, it will panic.
-func Register(scheme string, factory SchemeFactory) {
+// NewRegistry creates an independent snapshot of the globally registered
+// schemes, including cloud providers already registered through side-effect
+// imports. Future registrations in either registry do not affect the other.
+func NewRegistry() *Registry {
+ return defaultRegistry.Clone()
+}
+
+// Clone returns an independent copy of a registry's factory registrations.
+// Factories themselves are shared, but the registration maps and locks are
not.
+func (r *Registry) Clone() *Registry {
+ r.mu.RLock()
+ defer r.mu.RUnlock()
+
+ return &Registry{factories: maps.Clone(r.factories)}
+}
+
+// Register adds a scheme factory. Unlike MustRegister, it returns errors for
+// invalid or duplicate registrations rather than panicking.
+func (r *Registry) Register(scheme string, factory SchemeFactory) error {
if factory == nil {
- panic("io: Register factory is nil")
+ return ErrInvalidSchemeFactory
}
- regMutex.Lock()
- defer regMutex.Unlock()
+ r.mu.Lock()
+ defer r.mu.Unlock()
+
+ if _, exists := r.factories[scheme]; exists {
+ return fmt.Errorf("%w: %q", ErrSchemeAlreadyRegistered, scheme)
+ }
- if _, dup := defaultRegistry[scheme]; dup {
- panic("io: Register called twice for scheme " + scheme)
+ if r.factories == nil {
+ r.factories = make(map[string]SchemeFactory)
}
- defaultRegistry[scheme] = factory
+ r.factories[scheme] = factory
+
+ return nil
}
-// Unregister removes the requested scheme factory from the registry.
-func Unregister(scheme string) {
- regMutex.Lock()
- defer regMutex.Unlock()
- delete(defaultRegistry, scheme)
+// MustRegister registers a scheme or panics. It is intended for provider init
+// functions where failing to register a required backend is unrecoverable.
+func (r *Registry) MustRegister(scheme string, factory SchemeFactory) {
+ if err := r.Register(scheme, factory); err != nil {
+ panic(err)
+ }
}
-// GetRegisteredSchemes returns registered scheme names in sorted order.
-func GetRegisteredSchemes() []string {
- regMutex.RLock()
- defer regMutex.RUnlock()
+// Unregister removes a scheme from this registry only.
+func (r *Registry) Unregister(scheme string) {
+ r.mu.Lock()
+ defer r.mu.Unlock()
- return slices.Sorted(maps.Keys(defaultRegistry))
+ delete(r.factories, scheme)
}
-func init() {
- // Register local filesystem schemes
- localFSFactory := func(ctx context.Context, parsed *url.URL, props
map[string]string) (IO, error) {
- return LocalFS{}, nil
- }
- Register("file", localFSFactory)
- Register("", localFSFactory)
+// RegisteredSchemes returns the scheme names in sorted order.
+func (r *Registry) RegisteredSchemes() []string {
+ r.mu.RLock()
+ defer r.mu.RUnlock()
+
+ return slices.Sorted(maps.Keys(r.factories))
+}
+
+// Register adds a scheme to the global registry and returns an error for nil
+// factories or duplicate registrations. Use MustRegister from init functions.
+func Register(scheme string, factory SchemeFactory) error {
Review Comment:
Could `io.Register` keep `main`'s panic, or the error-returning version take
a new name so existing calls stop compiling? With this change a statement call
like `io.Register("myfs", f)` still compiles, but a duplicate keeps the first
factory and a nil one is dropped, with no panic or log. A repro with a
duplicate and a nil statement call logs `factory used=f1, fc-nil
registered=false`; [`main` panics on
both](https://github.com/apache/iceberg-go/blob/84c6fd693fe25bc9f6e3c9d99ef733930a6bc30f/io/registry.go#L41-L54).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
