zeroshade commented on code in PR #2335:
URL: https://github.com/apache/iceberg-go/pull/2335#discussion_r4390823150
##########
catalog/registry.go:
##########
@@ -76,30 +77,84 @@ func (f RegistrarFunc) GetCatalog(ctx context.Context,
catalogName string, props
return f(ctx, catalogName, props)
}
-// Register adds the new catalog type to the registry. If the catalog type is
already registered, it will be replaced.
-func Register(catalogType string, reg Registrar) {
+// Register adds a catalog factory to this registry. An existing catalog type
+// is replaced. An empty name or nil registrar returns ErrInvalidArgument.
+func (r *Registry) Register(catalogType string, reg Registrar) error {
+ if catalogType == "" {
+ return fmt.Errorf("%w: catalog registry type must not be
empty", iceberg.ErrInvalidArgument)
+ }
if reg == nil {
- panic("catalog: RegisterCatalog catalog factory is nil")
+ return fmt.Errorf("%w: catalog registrar must not be nil",
iceberg.ErrInvalidArgument)
+ }
+
+ r.mu.Lock()
+ defer r.mu.Unlock()
+
+ if r.registrars == nil {
+ r.registrars = make(map[string]Registrar)
+ }
+ r.registrars[strings.ToLower(catalogType)] = reg
+
+ return nil
+}
+
+// MustRegister registers a catalog factory or panics. Intended for package
init.
+func (r *Registry) MustRegister(catalogType string, reg Registrar) {
+ if err := r.Register(catalogType, reg); err != nil {
+ panic(err)
}
- defaultRegistry.set(strings.ToLower(catalogType), reg)
}
-// Unregister removes the requested catalog factory from the registry.
+// Unregister removes a catalog factory from this registry.
+func (r *Registry) Unregister(catalogType string) {
+ r.mu.Lock()
+ defer r.mu.Unlock()
+
+ delete(r.registrars, strings.ToLower(catalogType))
+}
+
+// GetRegisteredCatalogs lists catalog types in sorted order.
+func (r *Registry) GetRegisteredCatalogs() []string {
Review Comment:
The other two registries call this `Names()`. Could the method be `Names()`
too? The package-level `GetRegisteredCatalogs()` can stay as it is.
##########
catalog/registry.go:
##########
@@ -30,39 +30,40 @@ import (
"github.com/apache/iceberg-go/config"
)
-type registry map[string]Registrar
-
-func (r registry) getKeys() []string {
- regMutex.RLock()
- defer regMutex.RUnlock()
-
- return slices.Sorted(maps.Keys(r))
+// Registry holds catalog factories independently of the package-level default.
+// The zero value is ready to use, and its methods are safe for concurrent use.
+type Registry struct {
+ mu sync.RWMutex
+ registrars map[string]Registrar
}
-func (r registry) set(catalogType string, reg Registrar) {
- regMutex.Lock()
- defer regMutex.Unlock()
- r[catalogType] = reg
+// NewRegistry returns an empty registry. Importing a catalog backend registers
+// it with the package-level default, not with independently created
registries.
+// Use CloneDefaultRegistry to start with those registrations.
+func NewRegistry() *Registry {
Review Comment:
`io.NewRegistry()` and `encryption.NewKMSRegistry()` both return a snapshot
of the default registry, including backends registered by side-effect imports.
Returning an empty registry here means `reg.Load(ctx, "rest", ...)` fails
unless the caller knows to use `CloneDefaultRegistry()`. Could `NewRegistry()`
clone the default like the other two, and drop `CloneDefaultRegistry`?
##########
catalog/registry.go:
##########
@@ -76,30 +77,84 @@ func (f RegistrarFunc) GetCatalog(ctx context.Context,
catalogName string, props
return f(ctx, catalogName, props)
}
-// Register adds the new catalog type to the registry. If the catalog type is
already registered, it will be replaced.
-func Register(catalogType string, reg Registrar) {
+// Register adds a catalog factory to this registry. An existing catalog type
+// is replaced. An empty name or nil registrar returns ErrInvalidArgument.
+func (r *Registry) Register(catalogType string, reg Registrar) error {
+ if catalogType == "" {
+ return fmt.Errorf("%w: catalog registry type must not be
empty", iceberg.ErrInvalidArgument)
+ }
if reg == nil {
- panic("catalog: RegisterCatalog catalog factory is nil")
+ return fmt.Errorf("%w: catalog registrar must not be nil",
iceberg.ErrInvalidArgument)
+ }
+
+ r.mu.Lock()
+ defer r.mu.Unlock()
+
+ if r.registrars == nil {
+ r.registrars = make(map[string]Registrar)
+ }
+ r.registrars[strings.ToLower(catalogType)] = reg
+
+ return nil
+}
+
+// MustRegister registers a catalog factory or panics. Intended for package
init.
+func (r *Registry) MustRegister(catalogType string, reg Registrar) {
+ if err := r.Register(catalogType, reg); err != nil {
+ panic(err)
}
- defaultRegistry.set(strings.ToLower(catalogType), reg)
}
-// Unregister removes the requested catalog factory from the registry.
+// Unregister removes a catalog factory from this registry.
+func (r *Registry) Unregister(catalogType string) {
+ r.mu.Lock()
+ defer r.mu.Unlock()
+
+ delete(r.registrars, strings.ToLower(catalogType))
+}
+
+// GetRegisteredCatalogs lists catalog types in sorted order.
+func (r *Registry) GetRegisteredCatalogs() []string {
+ r.mu.RLock()
+ defer r.mu.RUnlock()
+
+ return slices.Sorted(maps.Keys(r.registrars))
+}
+
+// Register adds a factory to the process-wide default registry. An existing
+// catalog type is replaced. Invalid registrations return an error instead of
+// panicking. Use MustRegister for package initialization.
+func Register(catalogType string, reg Registrar) error {
Review Comment:
`io.Register` and `encryption.RegisterKMS` kept their signatures (still
panicking) and added `MustRegister`, so they aren't breaking. Making
package-level `catalog.Register` return an error is the only breaking part of
this PR. Could it keep its old behavior, with errors only from
`(*Registry).Register`?
--
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]
