You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2026/09/23 12:55:11 UTC
[PR] Extend `databasev1.Node` to support report file format versions and timezone [skywalking-banyandb]
mrproliu opened a new pull request, #1373:
URL: https://github.com/apache/skywalking-banyandb/pull/1373
## What this does
Adds two wire-additive fields to `databasev1.Node`, so a client that only talks to a liaison can
learn what each node in the cluster can read and which time grid it is on:
```protobuf
cluster.v1.VersionInfo version = 10;
string tz_name = 11;
```
Both are filled in `api/common.Node.ToProtoNode()`, so `NodeQueryService.GetCurrentNode` and the
`route_tables["tire2"].registered[]` of `ClusterStateService.GetClusterState` carry them with no
change at the three existing call sites. Nothing consumes them yet — this is step 1 of the
[data export/import design](docs/design/0.12.0/data-export-import/README.md), whose version and
time zone gates both need per-data-node facts that only each node knows.
## Changes
**Proto and its filling**
- `api/proto/banyandb/database/v1/database.proto`: the two fields, plus the `cluster/v1` import.
- `api/common/id.go`: `ToProtoNode` fills them. `banyand/backup/lifecycle` now builds its self-node
through `ToProtoNode` too, instead of a hand-rolled literal that would drift.
**`pkg/fileformat` (new leaf package)**
`api/common` cannot reach the segment file format version where it lived: `banyand/internal/storage`
sits under `banyand/internal/` and also imports `api/common`, so both the Go internal rule and a
cycle block it. Filling at the call sites does not work either — `storage`'s own dependency tree
contains `banyand/metadata/discovery/none`, one of the three. So `versions.yml` and the current
version move to `pkg/fileformat`, `storage.GetCurrentVersion()` / `GetCompatibleVersions()` forward
to it, and no caller changes. A `check-import-boundaries` rule keeps the new package a leaf.
**`pkg/host.TimeZoneName()` (new)**
Resolves the IANA name from the system, in the order each platform's own runtime resolves the zone:
- Unix: `TZ`, then the `/etc/localtime` symlink target, then the contents of that file when it is a
plain copy, then the zone the runtime itself settled on if it says `UTC`.
- Windows: `TimeZoneKeyName` from the registry, mapped through CLDR. It ignores `TZ` because Go's
Windows `initLocal` ignores it too, and it does not validate through `LoadLocation` because
Windows ships no zoneinfo database.
It never falls back to `time.Local.String()`: with `TZ` unset that returns the literal `"Local"`
whatever the zone is, so two nodes twelve hours apart would report the same name and an equality
check between them would pass. An unresolved zone reports empty, which means unknown, not match.
The Windows mapping table is generated from the Unicode CLDR `windowsZones.xml` (`territory="001"`,
139 entries) rather than written by hand, and compiles only into Windows builds.
**Embedded time zone database**
`banyand/internal/storage` imports `time/tzdata`. Resolving a zone by name needs a zone database and
the minimal images do not ship one — busybox, alpine and ubuntu all leave out `/usr/share/zoneinfo`.
Today the images this project publishes disagree: the distroless one carries a database, the busybox
ones do not, so with `TZ` set a data node and a lifecycle sidecar of the same cluster read the same
segment directory eight hours apart. Embedding settles it in the binary. It costs about 400KiB and
is not linked into `bydbctl`.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Part of apache/skywalking#14102.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend `databasev1.Node` to support report file format versions and timezone [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1373:
URL: https://github.com/apache/skywalking-banyandb/pull/1373#discussion_r4082797840
##########
pkg/host/timezone_unix.go:
##########
@@ -0,0 +1,138 @@
+// Licensed to 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. Apache Software Foundation (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.
+
+//go:build !windows
+
+package host
+
+import (
+ "os"
+ "path/filepath"
+ "strings"
+ "time"
+)
+
+// localtimePath is the symlink the Go runtime itself consults when TZ is unset.
+// It is a variable so tests can point it at a fixture.
+var localtimePath = "/etc/localtime"
+
+// runtimeZoneName reports the name the Go runtime gave the local zone. It is a
+// variable so tests can drive the branch that reads it without depending on the
+// zone the test runner happens to be in.
+var runtimeZoneName = time.Local.String
+
+// zoneinfoMarker is the last path element shared by every zoneinfo database
+// layout: /usr/share/zoneinfo on Linux, /var/db/timezone/zoneinfo on macOS.
+const zoneinfoMarker = "zoneinfo"
+
+// A zone that is UTC now but not always is not UTC. January and July of every
+// year in this range catch both daylight saving and the historical offset
+// changes that every non-UTC zone carries.
+const (
+ firstProbeYear = 1970
+ lastProbeYear = 2038
+)
+
+// localZoneName reads the zone the way the Go runtime does on this platform:
+// TZ when it is set, otherwise the /etc/localtime link.
+func localZoneName() string {
+ if tz, ok := os.LookupEnv("TZ"); ok {
+ // Read TZ the way the Go runtime does: an empty value means UTC, a leading
+ // colon is stripped, and an absolute path names a zone file rather than a
+ // zone, so the name has to be recovered from the path.
+ name := strings.TrimPrefix(tz, ":")
+ if name == "" {
+ return "UTC"
+ }
+ if strings.HasPrefix(name, "/") {
+ name = zoneNameFromPath(name)
+ }
+ return validZoneName(name)
+ }
+ // EvalSymlinks rather than Readlink: it follows a chain of links, and on a
+ // plain file it yields that file's own path, which carries no zone name.
+ if target, err := filepath.EvalSymlinks(localtimePath); err == nil {
+ if name := validZoneName(zoneNameFromPath(target)); name != "" {
+ return name
+ }
+ }
+ // No name anywhere in the path. Container images copy the zone file instead of
+ // linking it, so the next thing to read is the file's own contents.
+ if name := utcZoneName(localtimePath); name != "" {
+ return name
+ }
+ // Distroless images ship no /etc/localtime at all, so nothing on disk can
+ // answer. The runtime resolved a zone anyway, and initLocal names it "UTC"
+ // only on the branches where it gave up and used UTC — a zone it loaded by
+ // name keeps that name, and one loaded from /etc/localtime is called "Local".
+ // So when this reports UTC, UTC is what the process is really running.
+ if runtimeZoneName() == "UTC" {
+ return "UTC"
+ }
+ return ""
+}
+
+// utcZoneName reports "UTC" when the zone file at path is the UTC zone, and an
+// empty string otherwise.
+//
+// A copied zone file carries rules but no name, and rules cannot be reversed
+// into an IANA name in general because several zones share them. UTC is the one
+// case worth resolving: it is what the stock container images put in
+// /etc/localtime, and it is the zone whose offset is zero and whose abbreviation
+// is "UTC" at every instant — Africa/Abidjan is also permanently UTC+0 but calls
+// itself GMT, so the abbreviation keeps the two apart.
+func utcZoneName(path string) string {
+ data, err := os.ReadFile(path)
+ if err != nil {
+ return ""
+ }
+ loc, err := time.LoadLocationFromTZData("", data)
+ if err != nil {
+ return ""
+ }
+ for year := firstProbeYear; year <= lastProbeYear; year++ {
+ for _, month := range [...]time.Month{time.January, time.July} {
+ name, offset := time.Date(year, month, 1, 0, 0, 0, 0, time.UTC).In(loc).Zone()
+ if offset != 0 || name != "UTC" {
+ return ""
+ }
+ }
+ }
+ return "UTC"
+}
+
+// zoneNameFromPath extracts the part of a zoneinfo file path that follows the
+// zoneinfo directory, which is exactly the IANA name.
+func zoneNameFromPath(path string) string {
+ segments := strings.Split(filepath.Clean(path), "/")
+ for i := len(segments) - 1; i >= 0; i-- {
+ if segments[i] == zoneinfoMarker {
+ return strings.Join(segments[i+1:], "/")
+ }
+ }
+ return ""
+}
+
+func validZoneName(name string) string {
+ if name == "" {
+ return ""
+ }
+ if _, err := time.LoadLocation(name); err != nil {
+ return ""
+ }
+ return name
+}
Review Comment:
Time zone validation depends on time.LoadLocation(name), which requires a zoneinfo database. This PR embeds tzdata only in banyand/internal/storage, but host.TimeZoneName() is called from api/common.ToProtoNode() and may be used in processes that do not import storage; in minimal images, those processes would still report an empty tz_name even with TZ set. To make behavior consistent across all binaries that report tz_name, consider moving the tzdata blank-import into a package that is guaranteed to be linked by those binaries (or otherwise centralizing tzdb initialization), while still keeping it out of bydbctl if that constraint is required.
##########
pkg/fileformat/version.go:
##########
@@ -0,0 +1,64 @@
+// Licensed to 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. Apache Software Foundation (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 fileformat holds the on-disk segment file format version line. It is a
+// leaf package so that both the storage engine and the low-level node types in
+// api/common can report the same versions without importing each other.
+package fileformat
+
+import (
+ "embed"
+ "encoding/json"
+ "slices"
+
+ "sigs.k8s.io/yaml"
+)
+
+// CurrentVersion is the version string a freshly-written segment must carry.
+const CurrentVersion = "1.5.0"
+
+//go:embed versions.yml
+var versionFS embed.FS
+
+var compatibleVersions = readCompatibleVersions()
+
+// CompatibleVersions returns every segment file format version this build reads.
+// The result is a copy: callers hand it straight to proto messages, and one of
+// them appending to it would rewrite the list for the whole process.
+func CompatibleVersions() []string {
+ return slices.Clone(compatibleVersions)
+}
+
+func readCompatibleVersions() []string {
+ i, err := versionFS.ReadFile("versions.yml")
+ if err != nil {
+ panic(err)
+ }
+ j, err := yaml.YAMLToJSON(i)
+ if err != nil {
+ panic(err)
+ }
+ var versions map[string][]string
+ if err := json.Unmarshal(j, &versions); err != nil {
+ panic(err)
+ }
+ vv, ok := versions["versions"]
+ if !ok {
+ panic("versions not found")
+ }
+ return vv
+}
Review Comment:
The panics here (panic(err) / \"versions not found\") will be surfaced as-is and may be hard to triage when a build artifact is mispackaged or the embedded YAML format changes. Consider improving the panic messages to include context (e.g., which embedded file/key failed) so failures are actionable.
##########
pkg/host/timezone.go:
##########
@@ -0,0 +1,33 @@
+// Licensed to 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. Apache Software Foundation (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 host
+
+// TimeZoneName returns the IANA name of the host's local time zone, such as
+// "Asia/Shanghai", or an empty string when the real name cannot be determined.
+// An empty name means "unknown": callers comparing two hosts must treat it as a
+// mismatch rather than as agreement.
+//
+// There is no portable way to ask for this. The Go runtime resolves the local
+// zone but never exposes its name — time.Local.String() reports the literal
+// "Local" whenever TZ is unset, which is the normal state inside a container,
+// so two hosts in different zones would look identical. Each platform therefore
+// resolves the name the same way its own runtime resolves the zone: see
+// localZoneName in timezone_unix.go and timezone_windows.go.
+func TimeZoneName() string {
+ return localZoneName()
+}
Review Comment:
TimeZoneName() recomputes on every call, and the Unix implementation can include filesystem reads plus tzfile parsing and probing. Since ToProtoNode() is likely called frequently (e.g., registrations, responses), consider caching the resolved value (e.g., via sync.Once) to avoid repeated I/O and repeated tz parsing.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extend `databasev1.Node` to support report file format versions and timezone [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1373:
URL: https://github.com/apache/skywalking-banyandb/pull/1373
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org