Support node registry by DNS #920

mrproliu · 2025-12-31T17:55:02Z

Support enabling/disabling the DNS node registry. If disabled, it will still use etcd as storage.
Add a new gRPC protocol to support querying the self node info.
Wrap the oberverbility as a sub package(service) to avoid package dependency issue.

codecov-commenter · 2025-12-31T20:33:01Z

Codecov Report

❌ Patch coverage is 62.33766% with 174 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.39%. Comparing base (3530dd9) to head (f6230d3).
⚠️ Report is 92 commits behind head on main.

Files with missing lines	Patch %	Lines
banyand/metadata/dns/dns.go	79.25%	51 Missing and 16 partials ⚠️
banyand/metadata/client.go	26.66%	38 Missing and 6 partials ⚠️
banyand/queue/sub/server.go	0.00%	20 Missing ⚠️
banyand/metadata/dns/metrics.go	0.00%	17 Missing ⚠️
banyand/measure/svc_data.go	0.00%	4 Missing ⚠️
banyand/queue/sub/node.go	0.00%	4 Missing ⚠️
banyand/internal/storage/disk_monitor.go	0.00%	3 Missing ⚠️
banyand/measure/svc_liaison.go	0.00%	2 Missing ⚠️
banyand/stream/svc_liaison.go	0.00%	2 Missing ⚠️
banyand/internal/storage/metrics.go	50.00%	1 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
+ Coverage   45.97%   46.39%   +0.41%     
==========================================
  Files         328      370      +42     
  Lines       55505    56901    +1396     
==========================================
+ Hits        25520    26400     +880     
- Misses      27909    27977      +68     
- Partials     2076     2524     +448

Flag	Coverage Δ
banyand	`49.68% <62.47%> (?)`
bydbctl	`81.91% <ø> (?)`
fodc	`90.65% <ø> (?)`
pkg	`29.08% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

wu-sheng · 2026-01-01T02:02:53Z

Support enabling/disabling the DNS node registry. If disabled, it will still use etcd as storage.

Please update document about service discovery, and indicate which is the default option.

Copilot

Pull request overview

This PR adds DNS-based node discovery as an alternative to the existing etcd-based node registry, enabling BanyanDB to support DNS SRV-based service discovery (e.g., in Kubernetes environments). The implementation includes:

Refactoring of the observability package: moved implementation to a services subpackage to avoid circular dependencies
Factory interface changed from pointer type to interface for better flexibility
New DNS discovery service with comprehensive test coverage

Reviewed changes

Copilot reviewed 59 out of 59 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
banyand/observability/type.go	Defines Factory as interface instead of concrete type, moved implementation details to services package
banyand/observability/services/*.go	Moved observability implementation from parent package to services subpackage
banyand/metadata/dns/*.go	New DNS-based node discovery implementation with SRV record queries and gRPC node metadata fetching
banyand/queue/sub/node.go	New gRPC NodeQueryService endpoint to expose current node information
banyand/metadata/client.go	Integration of DNS discovery with configuration flags and lifecycle management
api/proto/banyandb/database/v1/rpc.proto	New NodeQueryService gRPC API definition
docs/api-reference.md	API documentation for the new NodeQueryService
Multiple test files	Updated imports to use observability/services package

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

banyand/queue/sub/server.go

banyand/metadata/client.go

banyand/metadata/dns/dns.go

banyand/metadata/client.go

banyand/metadata/dns/dns.go

Copilot

Pull request overview

Copilot reviewed 60 out of 60 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

banyand/queue/sub/server.go

banyand/metadata/dns/dns.go

banyand/metadata/client.go

banyand/metadata/dns/dns.go

Copilot · 2026-01-05T07:22:05Z

banyand/metadata/dns/dns.go

+		s.cacheMutex.RLock()
+		_, exists := s.nodeCache[addr]
+		s.cacheMutex.RUnlock()
+
+		if !exists {
+			// fetch node metadata from gRPC
+			node, fetchErr := s.fetchNodeMetadata(ctx, addr)
+			if fetchErr != nil {
+				s.log.Warn().
+					Err(fetchErr).
+					Str("address", addr).
+					Msg("Failed to fetch node metadata")
+				addErrors = append(addErrors, fetchErr)
+				continue
+			}
+
+			// update cache and notify handlers
+			s.cacheMutex.Lock()
+			s.nodeCache[addr] = node
+			s.cacheMutex.Unlock()


There's a potential race condition in the cache check. Between releasing the RLock on line 384 and acquiring the Lock on line 399, another goroutine could have added the same node to the cache. This could lead to duplicate fetchNodeMetadata calls and duplicate handler notifications for the same address. Consider using a check-lock-check pattern or using a single Lock for the entire operation.

I prefer the check-lock-check pattern

hanahmily · 2026-01-05T10:25:51Z

banyand/metadata/dns/dns.go

+		s.cacheMutex.RLock()
+		_, exists := s.nodeCache[addr]
+		s.cacheMutex.RUnlock()
+
+		if !exists {
+			// fetch node metadata from gRPC
+			node, fetchErr := s.fetchNodeMetadata(ctx, addr)
+			if fetchErr != nil {
+				s.log.Warn().
+					Err(fetchErr).
+					Str("address", addr).
+					Msg("Failed to fetch node metadata")
+				addErrors = append(addErrors, fetchErr)
+				continue
+			}
+
+			// update cache and notify handlers
+			s.cacheMutex.Lock()
+			s.nodeCache[addr] = node
+			s.cacheMutex.Unlock()


I prefer the check-lock-check pattern

hanahmily · 2026-01-05T10:48:26Z

banyand/metadata/dns/dns.go

+
+	// if there have any error occurred,
+	// then just return the query error to ignore the result to make sure the cache correct
+	if len(queryErrors) > 0 {


You should return resolved addresses, even if some cannot be resolved.

banyand/metadata/dns/dns.go

hanahmily · 2026-01-05T11:02:11Z

banyand/metadata/dns/dns_test.go

+			Expect(mockResolver.getCallCount("_grpc._tcp.test.local")).To(Equal(1))
+		})
+
+		It("should fail when any DNS query fails (🎯 critical scenario)", func() {


It should not fail when part of the DNS query fails.

banyand/metadata/client.go

Copilot

Pull request overview

Copilot reviewed 60 out of 60 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

banyand/metadata/dns/dns.go

banyand/queue/sub/node.go

Copilot · 2026-01-05T17:53:56Z

banyand/metadata/client.go

+	// Validate etcd endpoints (required when using etcd-based node discovery for schema storage)
+	if s.nodeDiscoveryMode == NodeDiscoveryModeEtcd && len(s.endpoints) == 0 {


DNS mode still requires etcd for schema storage, but the validation comment on line 145 says "required when using etcd-based node discovery for schema storage" which is misleading. When DNS mode is used, etcd endpoints are still required for schema operations. The validation should enforce that etcd endpoints are always required (regardless of discovery mode), or the comment should be clarified.

Suggested change

// Validate etcd endpoints (required when using etcd-based node discovery for schema storage)

if s.nodeDiscoveryMode == NodeDiscoveryModeEtcd && len(s.endpoints) == 0 {

// Validate etcd endpoints (always required for schema storage, regardless of node discovery mode)

if len(s.endpoints) == 0 {

It's a bug, pls fix it.

Copilot · 2026-01-05T17:53:56Z

banyand/metadata/client.go

+	if s.nodeDiscoveryMode == NodeDiscoveryModeDNS {
+		l.Info().Strs("srv-addresses", s.dnsSRVAddresses).Msg("Initializing DNS-based node discovery")
+
+		var createErr error
+		s.dnsDiscovery, createErr = dns.NewService(dns.Config{
+			SRVAddresses: s.dnsSRVAddresses,
+			InitInterval: s.dnsFetchInitInterval,
+			InitDuration: s.dnsFetchInitDuration,
+			PollInterval: s.dnsFetchInterval,
+			GRPCTimeout:  s.grpcTimeout,
+			TLSEnabled:   s.dnsTLSEnabled,
+			CACertPaths:  s.dnsCACertPaths,
+		})
+		if createErr != nil {
+			return fmt.Errorf("failed to create DNS discovery service: %w", createErr)
+		}
 	}


The DNS discovery service is created in PreRun but never started. Looking at the Serve method, s.dnsDiscovery.Start() is called there. However, if PreRun fails after creating the DNS discovery service (lines 219-230), the service and its associated resources (like TLS reloaders) are not cleaned up, leading to a resource leak.

Copilot · 2026-01-05T17:53:57Z

banyand/metadata/dns/dns.go

+
+// ListNode list all existing nodes from cache.
+func (s *Service) ListNode(ctx context.Context, role databasev1.Role) ([]*databasev1.Node, error) {
+	// if the service is haven't begun/finished, then try to query and update DNS first


The comment on line 550 has a grammar error: "haven't begun/finished" should be "hasn't begun/finished" since "service" is singular.

Suggested change

// if the service is haven't begun/finished, then try to query and update DNS first

// if the service hasn't begun/finished, then try to query and update DNS first

Copilot · 2026-01-05T17:53:57Z

docs/operation/node-discovery.md

+
+- Watches etcd prefix `/nodes/` for PUT (add/update) and DELETE events.
+- Revision-based streaming resumes from last known revision after reconnection.
+- Periodic full sync every 5-35 minutes (randomized) to detect missed events.


The documentation states that periodic full sync happens "every 5-35 minutes (randomized)" but the default value for --etcd-full-sync-interval flag is 30 minutes (non-randomized). This creates a discrepancy between the documentation and the actual implementation.

Support node registry by DNS

59d1330

mrproliu added this to the 0.10.0 milestone Dec 31, 2025

mrproliu requested a review from hanahmily December 31, 2025 17:55

mrproliu added the enhancement New feature or request label Dec 31, 2025

mrproliu added 3 commits January 1, 2026 01:55

Merge branch 'main' into dns-node-registry

b9d17e8

fix lint

8b3ef40

Fix lint

74385f0

mrproliu added 3 commits December 31, 2025 13:59

fix CI

702197c

fix CI

31dbb13

fix CI

2cc3764

wu-sheng requested a review from Copilot January 1, 2026 02:01

Copilot started reviewing on behalf of wu-sheng January 1, 2026 02:02 View session

Copilot AI reviewed Jan 1, 2026

View reviewed changes

hanahmily requested changes Jan 4, 2026

View reviewed changes

fix comment

27a3dd4

wu-sheng requested review from Copilot and hanahmily January 5, 2026 07:16

Copilot started reviewing on behalf of wu-sheng January 5, 2026 07:17 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

hanahmily requested changes Jan 5, 2026

View reviewed changes

mrproliu added 2 commits January 5, 2026 08:27

fix issue

cf85bfd

Merge branch 'main' into dns-node-registry

7990553

mrproliu requested a review from Copilot January 5, 2026 17:13

Copilot started reviewing on behalf of mrproliu January 5, 2026 17:39 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

wu-sheng requested a review from hanahmily January 6, 2026 00:16

mrproliu added 2 commits January 5, 2026 16:21

fix issue

6bd518f

fix issue

61591a3

mrproliu added 3 commits January 5, 2026 18:43

fix issue

63acf97

trying upgrade artifact version to fix SSL

0ef5f94

trying upgrade artifact version to fix SSL

f6230d3

		// Validate etcd endpoints (required when using etcd-based node discovery for schema storage)
		if s.nodeDiscoveryMode == NodeDiscoveryModeEtcd && len(s.endpoints) == 0 {

	// if the service is haven't begun/finished, then try to query and update DNS first
	// if the service hasn't begun/finished, then try to query and update DNS first

Support node registry by DNS #920

Are you sure you want to change the base?

Support node registry by DNS #920

Conversation

mrproliu commented Dec 31, 2025

Uh oh!

codecov-commenter commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wu-sheng commented Jan 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

hanahmily Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

hanahmily Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

hanahmily Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hanahmily Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

hanahmily Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

codecov-commenter commented Dec 31, 2025 •

edited

Loading