wolfeidau commented on code in PR #51: URL: https://github.com/apache/iceberg-go/pull/51#discussion_r1464148047
########## catalog/glue.go: ########## @@ -0,0 +1,162 @@ +// 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 ( + "context" + "errors" + "fmt" + + "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/table" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/glue" + "github.com/aws/aws-sdk-go-v2/service/glue/types" +) + +var ( + _ Catalog = (*GlueCatalog)(nil) +) + +type GlueAPI interface { + GetTable(ctx context.Context, params *glue.GetTableInput, optFns ...func(*glue.Options)) (*glue.GetTableOutput, error) + GetTables(ctx context.Context, params *glue.GetTablesInput, optFns ...func(*glue.Options)) (*glue.GetTablesOutput, error) +} + +type GlueCatalog struct { + glueSvc GlueAPI +} + +func NewGlueCatalog(awscfg aws.Config) *GlueCatalog { + return &GlueCatalog{ + glueSvc: glue.NewFromConfig(awscfg), + } +} Review Comment: @zeroshade The current model feels like it is following the factory method pattern https://en.wikipedia.org/wiki/Factory_method_pattern, which is great pattern for certain use cases, but in this case there are lots of other things to consider when creating these catalogs, like identity, custom platform specific configuration ect. I prefer the Go model which although using common interfaces, encourages a more explicit creation for these things as this avoids "magic", which is important for reliable, deterministic operation of systems. You still have common configuration/metrics/logging components, however these are instantiated based on the New routine in that catalog implementation. Really it comes down to migrations being more of a considered action in most Go libraries, this ensures the configuration and security of these implementations is respected. For example accessing an S3 data store could require specific role configuration for authentication, endpoints and event hooks, which may differ from the information used between different tables. A developer can do all this with the AWS SDK config, then pass it in when creating an IO for that particular store, and this is clearly specified in their code. That said the operation of the catalog using a common interface is great, this i where other common components interact and operate so it is nice to have. Again as I said before, this is just something I have observed as idiomatic in Go libraries, like the AWS SDK. -- 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
