ING-1352: Add custom name resolver #38
Conversation
Westwooo
changed the title
go.mod
Outdated
| go 1.19 | ||
| go 1.23.0 | ||
|
|
||
| toolchain go1.24.5 |
There was a problem hiding this comment.
Unsure this is compatible with the current go version in the gocb go.mod
There was a problem hiding this comment.
Have changed it to go 1.23.12 to match gocb's go.mod
Westwooo
force-pushed
the
ING-1352-custom-resolver
branch
2 times, most recently
from
a68247c to
11b8631
Compare
Westwooo
force-pushed
the
ING-1352-custom-resolver
branch
from
11b8631 to
ca23a16
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates from routing_v1 to routing_v2 and implements a custom DNS resolver with routing awareness for optimized connection management.
Key Changes:
- Replaces routing_v1 with routing_v2 across the codebase
- Adds a custom resolver that discovers all cluster nodes, fetches bucket routing information, and maintains watch streams for topology updates
- Removes unused routing watcher and topology management code
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| routingwatcher.go | Deleted unused routing watcher implementation |
| routingclient_topology.go | Removed topology translation and watching logic |
| routingclient.go | Updated to use routing_v2, removed bucket watcher management, added custom resolver registration |
| routingconn.go | Migrated from routing_v1 to routing_v2 client |
| resolver.go | New custom resolver that discovers nodes, lists buckets, and watches routing updates |
| impl_routing_v2.go | New routing_v2 implementation wrapper |
| impl_routing_v1.go | Deleted routing_v1 implementation wrapper |
| conn.go | Updated interface to use routing_v2 |
| go.mod | Updated Go version and dependencies including goprotostellar v1.0.3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| module github.com/couchbase/gocbcoreps | ||
|
|
||
| go 1.19 | ||
| go 1.23.0 |
There was a problem hiding this comment.
The Go version should specify only the major and minor version (e.g., 'go 1.23') rather than including the patch version. The Go toolchain ignores patch versions in go.mod files.
| go 1.23.0 | |
| go 1.23 |
resolver.go
Outdated
| } | ||
| } | ||
|
|
||
| func (r *customResolver) ResolveNow(resolver.ResolveNowOptions) {} |
There was a problem hiding this comment.
If this is optional then does actually specifying it change behaviour comparing to not defining it all? Is it actually required to be defined to match the interface def?
There was a problem hiding this comment.
It is required for the interface. The comment on the interface indicates implementing it is optional:
// ResolveNow will be called by gRPC to try to resolve the target name
// again. It's just a hint, resolver can ignore this if it's not necessary.
It seems to be called on startup to initialise the addresses and also when there is a need to reconnect/retry due to a failure. I've made a change to make resolveNow{} send a struct on a channel that we wait on in our watch loop so even if it is called multiple times concurrently we'll only be running one resolve thread at a time.
Westwooo
force-pushed
the
ING-1352-custom-resolver
branch
2 times, most recently
from
d7b66ce to
afa2604
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
resolver.go:1
- The error return value from the type assertion is being silently discarded. If the authenticator is not a BasicAuthenticator,
authwill be nil and could cause issues. Consider checking the ok value and handling non-BasicAuthenticator cases appropriately, or documenting why only BasicAuthenticator is supported.
package gocbcoreps
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1352-custom-resolver
branch
from
afa2604 to
60d350b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
resolver.go:1
- Silent failure on type assertion could lead to nil auth being passed to CustomResolverBuilder. If a non-BasicAuthenticator is provided, the resolver is registered with nil basicAuth without any warning or error, potentially causing unexpected behavior.
package gocbcoreps
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1352-custom-resolver
branch
from
60d350b to
6f79b49
Compare
| eps[i].Attributes = eps[i].Attributes.WithValue("numvbs", bucketToNumVBs) | ||
| } | ||
|
|
||
| if r.shouldUpdate.Load() { |
There was a problem hiding this comment.
Why is shouldUpdate a class-level atomic? Couldn't it just exist locally within this function?
| "google.golang.org/grpc/resolver" | ||
| ) | ||
|
|
||
| const optimizedRoutingScheme = "optimized" |
There was a problem hiding this comment.
Shouldn't this be couchbase2+optimized or couchbase2+dns or something?
There was a problem hiding this comment.
I mention it both because it seems like it should include the couchbase2 portion, and also because I'm suspicious of it being super long like this. I think it's expected that the user needs to be explicit in telling the SDK they want to establish multiple connections, but its probably fine that when there are multiple connections it automatically attempts to use optimize routing.
This PR adds a custom resolver that:
Also since we had to bump the goprotostellar version and routing_v1 was removed this PR removes all mentions of routing_v1 as well as some routing stuff that did not appear to be used.