laskoviymishka commented on code in PR #1680: URL: https://github.com/apache/iceberg-go/pull/1680#discussion_r3749988144
########## environment_context.go: ########## @@ -0,0 +1,82 @@ +// 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 iceberg + +import ( + "maps" + "sync" +) + +const ( + // EnvironmentContextEngineNameKey identifies the engine name property. + EnvironmentContextEngineNameKey = "engine-name" Review Comment: Since this is exported API and awkward to change once it ships, worth getting the name right now: `EnvironmentContextEngineNameKey` is a mouthful and the `Context` in the middle is structural noise. Java just uses `ENGINE_NAME`. I'd go with `EngineNameKey`/`EngineVersionKey`, or at least drop the `Context` — `EnvironmentEngineNameKey`. wdyt? ########## environment_context_test.go: ########## @@ -0,0 +1,94 @@ +// 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 iceberg + +import ( + "fmt" + "strconv" + "sync" + "testing" + + "github.com/stretchr/testify/assert" +) + +func preserveEnvironmentProperties(t *testing.T, keys ...string) { + t.Helper() + previous := EnvironmentContext() + t.Cleanup(func() { + for _, key := range keys { + if value, ok := previous[key]; ok { + SetEnvironmentProperty(key, value) + } else { + RemoveEnvironmentProperty(key) + } + } + }) +} + +func TestEnvironmentContextCopiesValues(t *testing.T) { + const key = "test-engine" + preserveEnvironmentProperties(t, key) + + SetEnvironmentProperty(key, "go") + context := EnvironmentContext() + context[key] = "changed" + + assert.Equal(t, "go", EnvironmentContext()[key]) + assert.Equal(t, Version(), EnvironmentContext()[environmentContextIcebergVersionKey]) + + RemoveEnvironmentProperty(key) + assert.NotContains(t, EnvironmentContext(), key) +} + +func TestEnvironmentContextAllowsIcebergVersionMutation(t *testing.T) { + preserveEnvironmentProperties(t, environmentContextIcebergVersionKey) + + SetEnvironmentProperty(environmentContextIcebergVersionKey, "custom") + assert.Equal(t, "custom", EnvironmentContext()[environmentContextIcebergVersionKey]) + + RemoveEnvironmentProperty(environmentContextIcebergVersionKey) + assert.NotContains(t, EnvironmentContext(), environmentContextIcebergVersionKey) +} + +func TestEnvironmentContextConcurrentAccess(t *testing.T) { + const workers = 8 + const iterations = 100 + + keys := make([]string, 0, workers) + for worker := range workers { + keys = append(keys, fmt.Sprintf("test-concurrent-%d", worker)) + } + preserveEnvironmentProperties(t, keys...) + + var wg sync.WaitGroup + for worker, key := range keys { + wg.Add(1) + go func(worker int, key string) { + defer wg.Done() + for iteration := range iterations { + SetEnvironmentProperty(key, strconv.Itoa(iteration)) + _ = EnvironmentContext()[key] Review Comment: This test proves the lock holds up under contention, which is worth having. But each worker uses its own key, and the `EnvironmentContext()` result here is read and discarded — so the thing that would actually be a bug, `EnvironmentContext()` handing back a live reference to the stored map instead of a clone, isn't exercised. I'd add a case where one goroutine calls `EnvironmentContext()` and then mutates the returned map while another goroutine `SetEnvironmentProperty`s the same key, run under `-race`. If the clone ever regressed to a live reference, that's what would catch it. ########## environment_context.go: ########## @@ -0,0 +1,82 @@ +// 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 iceberg + +import ( + "maps" + "sync" +) + +const ( + // EnvironmentContextEngineNameKey identifies the engine name property. + EnvironmentContextEngineNameKey = "engine-name" + // EnvironmentContextEngineVersionKey identifies the engine version property. + EnvironmentContextEngineVersionKey = "engine-version" + + environmentContextIcebergVersionKey = "iceberg-version" +) + +var environmentContext = struct { + sync.RWMutex + properties map[string]string +}{ + properties: make(map[string]string), +} + +var environmentContextInit sync.Once + +func initializeEnvironmentContext() { Review Comment: Could we replace the `sync.Once` + `initializeEnvironmentContext()` with a plain `func init()` that seeds the key? `Version()` is set by `init()` in `utils.go` in this same package, so ordering is fine, and today every `EnvironmentContext`/`Set`/`Remove` call pays a `Once.Do` check for a seed that's really just a package-load constant. It also cleans up a subtlety: once `Remove` deletes `iceberg-version`, the `Once` has already fired, so nothing re-seeds it. A plain `init()` makes "seeded at startup" the honest story. wdyt? ########## environment_context.go: ########## @@ -0,0 +1,82 @@ +// 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 iceberg + +import ( + "maps" + "sync" +) + +const ( + // EnvironmentContextEngineNameKey identifies the engine name property. + EnvironmentContextEngineNameKey = "engine-name" + // EnvironmentContextEngineVersionKey identifies the engine version property. + EnvironmentContextEngineVersionKey = "engine-version" + + environmentContextIcebergVersionKey = "iceberg-version" +) + +var environmentContext = struct { + sync.RWMutex + properties map[string]string +}{ + properties: make(map[string]string), +} + +var environmentContextInit sync.Once + +func initializeEnvironmentContext() { + environmentContextInit.Do(func() { + environmentContext.Lock() + environmentContext.properties[environmentContextIcebergVersionKey] = Version() Review Comment: `Version()` returns the bare Go module semver, or the literal `(unknown version)` when built from source — which is basically every dev/test/CI run. So in those environments the report advertises `(unknown version)` as the writer. Java seeds this key from `IcebergBuild.fullVersion()` (`Apache Iceberg X.Y.Z (commit …)`) and PyIceberg emits `PyIceberg X.Y.Z`. I'd format this as something like `Apache Iceberg Go <version>` so dashboards that key off `iceberg-version` to identify the client get a real, greppable value. A small helper in `utils.go` would do it. ########## table/scan_metrics.go: ########## @@ -106,6 +107,9 @@ func (acc *scanMetricsAccumulator) applyResultDeleteMetrics(tasks []FileScanTask // are left unset and omitted. func (scan *Scan) buildScanReport(acc *scanMetricsAccumulator, schema, projected *iceberg.Schema, planning time.Duration) metrics.ScanReport { ids, names := projectedFields(projected) + metadata := make(map[string]string, len(scan.options)+1) + maps.Copy(metadata, scan.options) + maps.Copy(metadata, iceberg.EnvironmentContext()) Review Comment: Copying `EnvironmentContext()` second means env context wins on key collisions, so a per-scan `engine-name` passed in `scan.options` gets silently overwritten by the process-wide value. That matches Java's ordering (`options()` first, then context), so I think it's the right call — but it's non-obvious enough that I'd drop a one-line comment saying env context intentionally takes precedence. While you're here: `EnvironmentContext()` takes an RLock and clones on each call, and the `len(scan.options)+1` hint under-counts when there's more than one env key. I'd hoist it into a local — `envCtx := iceberg.EnvironmentContext()` — and size the map with `len(scan.options)+len(envCtx)`. ########## table/commit_metrics_test.go: ########## @@ -115,6 +115,36 @@ func TestBuildCommitReport(t *testing.T) { assert.Nil(t, m.AddedDVs) } +func TestBuildCommitReportIncludesEnvironmentContext(t *testing.T) { + keys := []string{ + iceberg.EnvironmentContextEngineNameKey, + iceberg.EnvironmentContextEngineVersionKey, + } + previous := iceberg.EnvironmentContext() Review Comment: This save/restore block is now copy-pasted here and in `scan_metrics_test.go`, and it's a near-duplicate of `preserveEnvironmentProperties` in the root package (which the table tests can't import since it's in package `iceberg`). Worth exporting a small test helper — `iceberg.PreserveEnvironmentProperties(t, keys...)` — or re-adding a bulk `SetEnvironmentContext(map)` so cleanup is one call. Otherwise the three copies drift when the reset logic changes. ########## environment_context.go: ########## @@ -0,0 +1,82 @@ +// 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 iceberg + +import ( + "maps" + "sync" +) + +const ( + // EnvironmentContextEngineNameKey identifies the engine name property. + EnvironmentContextEngineNameKey = "engine-name" + // EnvironmentContextEngineVersionKey identifies the engine version property. + EnvironmentContextEngineVersionKey = "engine-version" + + environmentContextIcebergVersionKey = "iceberg-version" +) + +var environmentContext = struct { + sync.RWMutex + properties map[string]string +}{ + properties: make(map[string]string), +} + +var environmentContextInit sync.Once + +func initializeEnvironmentContext() { + environmentContextInit.Do(func() { + environmentContext.Lock() + environmentContext.properties[environmentContextIcebergVersionKey] = Version() + environmentContext.Unlock() + }) +} + +// EnvironmentContext returns an independent snapshot of the process-wide +// context used to populate report metadata. The returned map may be modified +// by the caller without changing the stored context. +func EnvironmentContext() map[string]string { + initializeEnvironmentContext() + + environmentContext.RLock() + defer environmentContext.RUnlock() + + return maps.Clone(environmentContext.properties) +} + +// SetEnvironmentProperty sets one process-wide environment context property. +func SetEnvironmentProperty(key, value string) { + initializeEnvironmentContext() + + environmentContext.Lock() + defer environmentContext.Unlock() + + environmentContext.properties[key] = value +} + +// RemoveEnvironmentProperty removes one process-wide environment context +// property. +func RemoveEnvironmentProperty(key string) { Review Comment: One thing I'd like us to settle: `iceberg-version` is fully mutable and removable through these setters, and `TestEnvironmentContextAllowsIcebergVersionMutation` locks that in. A single `RemoveEnvironmentProperty("iceberg-version")` from app code or an adapter silently drops the version from every subsequent report, and there's no re-seed path. Java's `ConcurrentHashMap` is unguarded too, so freely-mutable does match Java. But I lean toward protecting this one key — either no-op the setters for it, or keep it as a separate field that's always merged in at read time in `EnvironmentContext()` — so a stray delete can't blank it out everywhere. If we keep it mutable, I'd at least add a line to this doc noting that removing `iceberg-version` drops it permanently. wdyt? ########## environment_context.go: ########## @@ -0,0 +1,82 @@ +// 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 iceberg + +import ( + "maps" + "sync" +) + +const ( + // EnvironmentContextEngineNameKey identifies the engine name property. + EnvironmentContextEngineNameKey = "engine-name" + // EnvironmentContextEngineVersionKey identifies the engine version property. + EnvironmentContextEngineVersionKey = "engine-version" + + environmentContextIcebergVersionKey = "iceberg-version" +) + +var environmentContext = struct { + sync.RWMutex Review Comment: Minor, but the rest of the repo uses a named mutex field (`registryMu`, `regMutex`, `mu`) rather than embedding. Embedding promotes `Lock`/`RLock`/… onto the package var, so package code can bypass the public API with `environmentContext.Lock()`, and it trips `copylocks` if the struct ever gets copied. I'd make it `mu sync.RWMutex` and use `environmentContext.mu.Lock()`. -- 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]
