Repository navigation
Fixing the fallback balancer deprioritizing the wrong way - #32
Open
AnomalRoil wants to merge 13 commits into
Open
AnomalRoil wants to merge 13 commits into
AnomalRoil wants to merge 13 commits into
Conversation
Nodes are picked by ascending priority value, so subtracting from a failing node's priority made it more preferred. A failed call now moves its node to the back, except when the caller gave up or asked for something no node can serve. With that, the Next stream no longer needs to skip on reopen.
…needs The shared Next stream now marks its calls with a penalty that outlasts its stall timeout and backoff, so the balancer's priority reset no longer sends a reopen back to the node that just failed it. Unary calls only move a node on transport-level errors, since drand nodes report unknown chains and rounds as codes.Unknown. Picker rebuilds keep existing penalties.
…lty with the node count A Next stream that fails with an error is reopened on the next node while callers wait, until every node failed it once. The stream penalty now covers the stall of every other node, and a shorter penalty no longer ends a longer one early.
Nodes are kept per address so a penalty survives reconnections, and Pick reads a cached choice that refreshes when a penalty ends.
GetHealth now names the node to avoid, penalized once however often it is picked. Stalls stop penalizing nodes once the stream stalled on every node. Invalid UTF-8 beacon IDs are rejected before any gRPC call.
AvoidNode no longer penalizes: /health cannot demote a node. A halted chain stays excused when its stream stops between stalls.
The /rounds/next stream now avoids the nodes that failed it since the last beacon instead of penalizing them for every chain. Only transport errors penalize a node, for streams and unary calls alike. AvoidNode becomes AvoidNodes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since nodes are picked by ascending priority value,
dec()subtracting 2 from a failing node was actually making it more preferred, so we never moved off a failing node.While fixing it I ended up rewriting the balancer: nodes are keyed by address and ranked by a single pure
rank()function, the first configured node stays preferred, and a node failing with a transport error (Unavailable,DeadlineExceeded, etc.) is put behind the others forFallbackTimeout. There are no more priorities, ticker orLBConfig. Plain errors from a node (unknown round, unknown chain, etc.) and callers giving up don't penalize anything, since otherwise any client could make us flap.The
Nextstream from #31 doesn't touch that ranking anymore: each chain's stream keeps the list of nodes that failed it since the last beacon and asks the picker to avoid them withAvoidNodes, so a stuck node or a halted chain only affects its own stream, and every node gets tried before we go back to one.SkipCtxKeyand the alternating skip are gone.Also,
GetBeaconnow does its single retry on another node,/healthchecks the next node when the first one is behind without demoting it, and beacon IDs that aren't valid UTF-8 are rejected before reaching gRPC.Notice a node that answers but lags is not demoted for
/public/latest, only skipped by/healthand by the retry.Also bumping version to v2.2.4.