laskoviymishka commented on code in PR #2075:
URL: https://github.com/apache/iceberg-go/pull/2075#discussion_r4153186787
##########
catalog/rest/rest.go:
##########
@@ -1329,6 +1331,33 @@ func (r *Catalog) fetchTableCreds(ctx context.Context,
ident []string, location
return resolveStorageCredentials(ret.StorageCredentials, location), nil
}
+// RefreshTableCredentials updates a *table.Table with newly-vended
credentials from the catalog
+// without updating any other table-internal state that a full Refresh() would.
+// Allows for a quick table credential refresh if the table was created
without any pre-seeded
+// credentials.
+//
+// Requires that the passed-in table instance be created with the
table.WithSavedConfig() option to
+// save any table-specific configs. All tables created by this catalog pass in
that option.
+func (r *Catalog) RefreshTableCredentials(ctx context.Context, tbl
*table.Table) (*table.Table, error) {
+ metadataLoc := tbl.MetadataLocation()
+ resp, err := r.fetchTableCreds(ctx, tbl.Identifier(), metadataLoc)
+ if err != nil {
+ return nil, err
+ }
+ if len(resp) == 0 {
+ // No new credentials vended. Return as-is.
+ return tbl, nil
Review Comment:
This returns the caller's own pointer rather than a new instance, and for an
externally-built table that pointer has no refresher wiring, so a server that
correctly vends nothing (IAM-role storage, say) hands back exactly the un-wired
table we set out to fix, with no signal that nothing was established. The
interface promises "a new/modified instance" that "can refresh its own
credentials internally," and this path delivers neither. I'd rebuild through
`tableFromResponse` even when `resp` is empty so the identity is always a
fresh, wired object. If the aliasing is deliberate, let's document it and
assert it in `TestRefreshTableCredentialsNoCredentialsVended`, since the
equality checks there pass trivially today (same object vs itself).
##########
table/table.go:
##########
@@ -133,6 +134,8 @@ func (t Table) Schema() *iceberg.Schema
{ return t.metadata
func (t Table) Spec() iceberg.PartitionSpec { return
t.metadata.PartitionSpec() }
func (t Table) SortOrder() SortOrder { return
t.metadata.SortOrder() }
func (t Table) Properties() iceberg.Properties { return
t.metadata.Properties() }
+func (t Table) ScanPlanningConfig() iceberg.Properties { return
t.scanPlanningIOProps }
+func (t Table) SavedConfig() iceberg.Properties { return
t.savedConfig }
Review Comment:
I'd clone on read here. Both accessors hand back the internal map directly,
so `tbl.SavedConfig()["k"] = v` mutates the table's stored field in place.
That's sharp now that `savedConfig` carries the vended creds: a caller poking
at the returned map corrupts the credential set `RefreshTableCredentials`
rebuilds FileIO from. `Identifier()` already clones on read, and every write
path here clones (`maps.Clone` in the `With*` options, `Refresh`, `doCommit`),
so clone-on-read is the consistent move. `maps.Clone(nil)` is nil, so
non-mutating callers don't change behavior.
```go
func (t Table) ScanPlanningConfig() iceberg.Properties { return
maps.Clone(t.scanPlanningIOProps) }
func (t Table) SavedConfig() iceberg.Properties { return
maps.Clone(t.savedConfig) }
```
##########
table/table.go:
##########
@@ -1400,6 +1405,20 @@ func WithScanPlanningIOProperties(props
iceberg.Properties) Option {
}
}
+// WithSavedConfig supplies a set of properties used to create a *Table
+// instance. This saved config is exposed through table.SavedConfig() and
+// can be used by callers to save properties useful for loading files of a
+// table, such as client factories and resolved storage credentials, so that
+// new clones of this table can contain the same settings.
+func WithSavedConfig(config iceberg.Properties) Option {
+ if config == nil {
+ return noopTableOption
+ }
+ return func(t *Table) {
Review Comment:
`nlreturn` wants a blank line before this `return`. Both sibling options
(`WithMetricsReporter`, `WithScanPlanningIOProperties`) have it and the
linter's active in `.golangci.yml`, so this trips CI. Blank line after the
nil-guard's closing brace.
--
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]
