bocharov commented on code in PR #400:
URL:
https://github.com/apache/kvrocks-controller/pull/400#discussion_r3716660704
##########
store/cluster_shard.go:
##########
@@ -420,6 +420,12 @@ func (shard *Shard) ToSlotsString() (string, error) {
}
for i, node := range shard.Nodes {
+ // A blank host (":port") would emit a malformed line and
register a phantom, unreachable node.
+ // Fail loudly here rather than push corruption to every
kvrocks node.
+ host, port, err := net.SplitHostPort(node.Addr())
+ if err != nil || host == "" || port == "" {
+ return "", fmt.Errorf("node %s has invalid address %q",
node.ID(), node.Addr())
+ }
Review Comment:
Done — added a shared `validateAddr(addr string) error` in
`store/cluster_shard.go`, now used by both `Shard.ToSlotsString` and
`ClusterStore.CheckNewNodes`.
##########
store/cluster_node.go:
##########
@@ -315,17 +330,52 @@ func (n *ClusterNode) GetClusterNodesString(ctx
context.Context) (string, error)
return strings.TrimRight(clusterNodesStr, "\n"), nil
}
-func (n *ClusterNode) SyncClusterInfo(ctx context.Context, cluster *Cluster)
error {
+// syncMaxRetries and syncRetryBaseDelay bound the per-node push retry.
kvrocks nodes do not gossip,
+// so the controller is the sole topology source; a push dropped by a
transient blip (node restarting,
+// network hiccup) is never re-tried by any other path until the next probe
tick, leaving that node on a
+// stale/divergent view (apache/kvrocks-controller#395). A short bounded
backoff closes that window.
+const (
+ syncMaxRetries = 3
+ syncRetryBaseDelay = 150 * time.Millisecond
+)
+
+// SyncClusterInfo pushes the authoritative topology to this node via CLUSTERX
SETNODEID + SETNODES.
+//
+// When force is true the SETNODES carries the `force` flag, which makes
kvrocks apply the topology
+// unconditionally — bypassing its version gate (reject-if-lower,
no-op-if-equal). This is what lets the
+// controller REPAIR a node that has drifted at an equal epoch (the divergence
#395 describes): a normal
+// equal-version push is silently no-op'd by the server, so it can never fix a
bad-but-same-version view.
+// Because SETNODES replaces the node's entire topology, a forced push also
removes stale/phantom node
+// entries (e.g. empty-address ghosts) without the data-destroying CLUSTER
RESET the alternative requires.
+// Safe because the controller is the single writer of topology; callers must
not force a version older
+// than a node legitimately ahead of the store (the probe loop keeps the `node
ahead` branch for that).
+func (n *ClusterNode) SyncClusterInfo(ctx context.Context, cluster *Cluster,
force bool) error {
Review Comment:
Done — replaced the `force bool` + package consts with a
`SyncPolicy{MaxRetries, RetryDelay, Force}` struct. `SyncClusterInfo` now takes
the policy; added `DefaultSyncPolicy()` and `ForceSyncPolicy()` helpers, and
callers use the latter.
##########
controller/cluster.go:
##########
@@ -242,12 +243,38 @@ func (c *ClusterChecker) syncClusterToNodes(ctx
context.Context) error {
return nil
}
+// topologyDiverged reports whether a probed node's applied topology disagrees
with the desired one:
+// a wrong peer count or wrong slot coverage. It returns false when the node
did not report these
+// fields (KnownNodes/SlotsOk < 0), so an older kvrocks that omits them falls
back to epoch-only
+// reconcile instead of being force-pushed on every tick.
+func topologyDiverged(info *store.ClusterInfo, wantNodes, wantSlots int64)
bool {
+ if info == nil || info.KnownNodes < 0 || info.SlotsOk < 0 {
+ return false
+ }
+ return info.KnownNodes != wantNodes || info.SlotsOk != wantSlots
+}
+
Review Comment:
Done — moved it onto `store.ClusterInfo` as
`(*ClusterInfo).TopologyDiverged(wantNodes, wantSlots)`; the reconcile loop now
calls `info.TopologyDiverged(...)` and the test moved to the store package.
--
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]
