Copilot commented on code in PR #971:
URL: https://github.com/apache/dubbo-go-pixiu/pull/971#discussion_r3371168336
##########
pkg/cluster/loadbalancer/load_balancer.go:
##########
@@ -63,20 +64,45 @@ type SnapshotLoadBalancer interface {
HandlerWithSnapshot(c PickContext, policy model.LbPolicy)
*model.Endpoint
}
-// HealthyOnlySnapshotLoadBalancer marks snapshot-aware balancers that do not
-// need PickContext.AllEndpoints. Unmarked snapshot balancers keep receiving
-// the full snapshot for compatibility with custom implementations.
+// HealthyOnlySnapshotLoadBalancer is retained for source compatibility with
+// callers that referenced the old marker contract.
+//
+// Deprecated: runtime fast-path decisions ignore this interface. Only trusted
+// in-tree balancers can opt in via SnapshotOptIn returning snapshotopt.Token.
type HealthyOnlySnapshotLoadBalancer interface {
UseHealthyEndpointsOnly() bool
}
-// ZeroCopySnapshotLoadBalancer marks trusted balancers that never mutate or
-// retain snapshot endpoints. Other snapshot balancers receive defensive
-// copies.
+// ZeroCopySnapshotLoadBalancer is retained for source compatibility with
+// callers that referenced the old marker contract.
+//
+// Deprecated: runtime fast-path decisions ignore this interface. Only trusted
+// in-tree balancers can opt in via SnapshotOptIn returning snapshotopt.Token.
type ZeroCopySnapshotLoadBalancer interface {
UseZeroCopySnapshot() bool
}
Review Comment:
PR description says the two exported marker interfaces were removed, but the
code keeps `HealthyOnlySnapshotLoadBalancer` and `ZeroCopySnapshotLoadBalancer`
(now marked Deprecated) for source compatibility. Please either update the PR
description to match the implementation, or remove these exported interfaces if
the intent is truly to delete them from the public surface.
##########
pkg/cluster/loadbalancer/ringhash/ring_hash.go:
##########
@@ -58,6 +59,13 @@ func NewRingHash(config model.ConsistentHash, endpoints
[]*model.Endpoint) model
type RingHashing struct{}
+var _ loadbalancer.HealthyOnlySnapshotLoadBalancer = RingHashing{}
+var _ loadbalancer.ZeroCopySnapshotLoadBalancer = RingHashing{}
Review Comment:
These compile-time assertions reference deprecated marker interfaces that no
longer affect runtime behavior. Keeping them introduces new, unnecessary
coupling to deprecated APIs. Prefer removing these `var _
...SnapshotLoadBalancer` assertions and rely on `SnapshotOptIn()` for the
fast-path opt-in.
##########
pkg/cluster/loadbalancer/rand/load_balancer_rand.go:
##########
@@ -32,6 +33,13 @@ func init() {
type Rand struct{}
+var _ loadbalancer.HealthyOnlySnapshotLoadBalancer = Rand{}
+var _ loadbalancer.ZeroCopySnapshotLoadBalancer = Rand{}
Review Comment:
These compile-time assertions reference deprecated marker interfaces that no
longer affect runtime behavior. Keeping them introduces new, unnecessary
coupling to deprecated APIs. Prefer removing these `var _
...SnapshotLoadBalancer` assertions and rely on `SnapshotOptIn()` for the
fast-path opt-in.
##########
pkg/cluster/loadbalancer/weightrandom/weight_random.go:
##########
@@ -40,6 +41,13 @@ type weightedEndpoint struct {
// It assigns weights to endpoints and uses these weights to influence the
probability of selection.
type WeightRandom struct{}
+var _ loadbalancer.HealthyOnlySnapshotLoadBalancer = WeightRandom{}
+var _ loadbalancer.ZeroCopySnapshotLoadBalancer = WeightRandom{}
Review Comment:
These compile-time assertions reference deprecated marker interfaces that no
longer affect runtime behavior. Keeping them introduces new, unnecessary
coupling to deprecated APIs. Prefer removing these `var _
...SnapshotLoadBalancer` assertions and rely on `SnapshotOptIn()` for the
fast-path opt-in.
##########
pkg/cluster/loadbalancer/roundrobin/round_robin.go:
##########
@@ -32,6 +33,13 @@ func init() {
type RoundRobin struct{}
+var _ loadbalancer.HealthyOnlySnapshotLoadBalancer = RoundRobin{}
+var _ loadbalancer.ZeroCopySnapshotLoadBalancer = RoundRobin{}
Review Comment:
These compile-time assertions reference deprecated marker interfaces that no
longer affect runtime behavior. Keeping them introduces new, unnecessary
coupling to deprecated APIs. Prefer removing these `var _
...SnapshotLoadBalancer` assertions and rely on `SnapshotOptIn()` for the
fast-path opt-in.
##########
pkg/cluster/loadbalancer/load_balancer.go:
##########
@@ -63,20 +64,45 @@ type SnapshotLoadBalancer interface {
HandlerWithSnapshot(c PickContext, policy model.LbPolicy)
*model.Endpoint
}
-// HealthyOnlySnapshotLoadBalancer marks snapshot-aware balancers that do not
-// need PickContext.AllEndpoints. Unmarked snapshot balancers keep receiving
-// the full snapshot for compatibility with custom implementations.
+// HealthyOnlySnapshotLoadBalancer is retained for source compatibility with
+// callers that referenced the old marker contract.
+//
+// Deprecated: runtime fast-path decisions ignore this interface. Only trusted
+// in-tree balancers can opt in via SnapshotOptIn returning snapshotopt.Token.
type HealthyOnlySnapshotLoadBalancer interface {
UseHealthyEndpointsOnly() bool
}
-// ZeroCopySnapshotLoadBalancer marks trusted balancers that never mutate or
-// retain snapshot endpoints. Other snapshot balancers receive defensive
-// copies.
+// ZeroCopySnapshotLoadBalancer is retained for source compatibility with
+// callers that referenced the old marker contract.
+//
+// Deprecated: runtime fast-path decisions ignore this interface. Only trusted
+// in-tree balancers can opt in via SnapshotOptIn returning snapshotopt.Token.
type ZeroCopySnapshotLoadBalancer interface {
UseZeroCopySnapshot() bool
}
+// snapshotOptInBalancer is the internal opt-in surface for trusted, in-tree
+// snapshot balancers. The method returns snapshotopt.Token, whose type lives
in
+// an internal package, so only balancers under pkg/cluster/loadbalancer can
+// implement this interface. External plugins cannot name the return type and
+// therefore always fall through to the safe default (full snapshot,
defensively
+// copied). This is the trust boundary described in issue #941.
+type snapshotOptInBalancer interface {
+ SnapshotOptIn() snapshotopt.Token
+}
Review Comment:
The new trust boundary (“external plugins cannot satisfy SnapshotOptIn
because they can’t name snapshotopt.Token”) can be bypassed by *embedding* an
in-tree balancer type that already has `SnapshotOptIn() snapshotopt.Token`.
Method promotion would make the external type satisfy `snapshotOptInBalancer`,
enabling the zero-copy / healthy-only fast paths for untrusted code.
Consider hardening `snapshotOptIn` with an additional runtime trust check
(e.g., verify the concrete balancer’s `reflect.Type.PkgPath()` is under the
in-tree `.../pkg/cluster/loadbalancer` prefix) before honoring the token, and
update the comment to reflect the actual guarantee.
##########
pkg/cluster/loadbalancer/maglev/maglev_hash.go:
##########
@@ -50,6 +51,13 @@ func NewMaglevHash(config model.ConsistentHash, endpoints
[]*model.Endpoint) mod
type MaglevHash struct{}
+var _ loadbalancer.HealthyOnlySnapshotLoadBalancer = MaglevHash{}
+var _ loadbalancer.ZeroCopySnapshotLoadBalancer = MaglevHash{}
Review Comment:
These compile-time assertions reference deprecated marker interfaces that no
longer affect runtime behavior. Keeping them introduces new, unnecessary
coupling to deprecated APIs. Prefer removing these `var _
...SnapshotLoadBalancer` assertions and rely on `SnapshotOptIn()` for the
fast-path opt-in.
--
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]
