Fokko commented on code in PR #244:
URL: https://github.com/apache/iceberg-go/pull/244#discussion_r1911238543
##########
catalog/glue.go:
##########
@@ -47,13 +50,65 @@ const (
// The ID of the Glue Data Catalog where the tables reside. If none is
provided, Glue
// automatically uses the caller's AWS account ID by default.
// See:
https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-catalog-databases.html
- glueCatalogIdKey = "glue.id"
+ GlueCatalogIdKey = "glue.id"
Review Comment:
All in line with the rest of the implementations 👍
##########
catalog/registry.go:
##########
@@ -0,0 +1,135 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package catalog
+
+import (
+ "fmt"
+ "maps"
+ "net/url"
+ "slices"
+ "strings"
+ "sync"
+
+ "github.com/apache/iceberg-go"
+)
+
+type registry map[string]Registrar
+
+func (r registry) getKeys() []string {
+ regMutex.Lock()
+ defer regMutex.Unlock()
+ return slices.Collect(maps.Keys(r))
+}
+
+func (r registry) set(catalogType string, reg Registrar) {
+ regMutex.Lock()
+ defer regMutex.Unlock()
+ r[catalogType] = reg
+}
+
+func (r registry) get(catalogType string) (Registrar, bool) {
+ regMutex.Lock()
+ defer regMutex.Unlock()
+ reg, ok := r[catalogType]
+ return reg, ok
+}
+
+func (r registry) remove(catalogType string) {
+ regMutex.Lock()
+ defer regMutex.Unlock()
+ delete(r, catalogType)
+}
+
+var (
+ regMutex sync.Mutex
+ defaultRegistry = registry{}
+)
+
+// Registrar is a factory for creating Catalog instances, used for registering
to use
+// with LoadCatalog.
+type Registrar interface {
+ GetCatalog(catalogURI string, props iceberg.Properties) (Catalog, error)
+}
+
+type RegistrarFunc func(string, iceberg.Properties) (Catalog, error)
+
+func (f RegistrarFunc) GetCatalog(catalogURI string, props iceberg.Properties)
(Catalog, error) {
+ return f(catalogURI, 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) {
+ if reg == nil {
+ panic("catalog: RegisterCatalog catalog factory is nil")
+ }
+ defaultRegistry.set(catalogType, reg)
+}
+
+// Unregister removes the requested catalog factory from the registry.
+func Unregister(catalogType string) {
+ defaultRegistry.remove(catalogType)
+}
+
+// GetRegsisteredCatalogs returns the list of registered catalog names that can
+// be looked up via LoadCatalog.
+func GetRegisteredCatalogs() []string {
+ return defaultRegistry.getKeys()
+}
+
+// Load allows loading a specific catalog by URI and properties.
+//
+// This is utilized alongside RegisterCatalog/UnregisterCatalog to not only
allow
+// easier catalog loading but also to allow for custom catalog implementations
to
+// be registered and loaded external to this module.
+//
+// The URI is used to determine the catalog type by first checking if it
contains
+// the string "://" indicating the presence of a scheme. If so, the scheme is
used
+// to lookup the registered catalog. i.e. "glue://..." would return the Glue
catalog
+// implementation, passing the URI and properties to NewGlueCatalog. If no
scheme is
+// present, then the URI is used as-is to lookup the catalog factory function.
Review Comment:
For the other implementations we rely on the `type` to load the correct
implementation, if this one isn't present, we fall back to the URL to see if we
can derive the type (`thrift://` for Hive, etc). I would suggest following this
in Go as well. The logic below it more like the other way around, where the
`URI` will fallback to type.
The following would be more in line with the other implementations:
```go
catalog.Load(iceberg.Properties{"type": "rest", "uri": "http://foobar"})
```
I think the names make it easy to refer to the different catalogs (eg. prod,
staging, etc), and might also be interesting to copy to Go as well.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
