MOD-16382/16399 Unified topology reconcile on de-globalized structure (#104 + #105) - #106
Draft
gabsow wants to merge 12 commits into
Draft
MOD-16382/16399 Unified topology reconcile on de-globalized structure (#104 + #105)#106gabsow wants to merge 12 commits into
gabsow wants to merge 12 commits into
Conversation
Expose MR_ClusterRefreshTopology(): refreshes the LibMR cluster view in response to a Redis server event instead of requiring a manual REFRESHCLUSTER on every primary. Gated by clusterCtx.isOss so it is a no-op outside OSS cluster mode. A reshard issues CLUSTER SETSLOT per slot, so the topology-change event can fire thousands of times in a burst; refreshing per event would tear down and rebuild every inter-shard connection thousands of times (MR_RefreshClusterData frees the whole cluster). So the refresh is trailing-debounced via an event-loop task: each event only bumps a counter, and a single refresh runs once the counter has been stable for one debounce window. Verified on a live 4-shard reshard: ~8000 events collapse to 1-2 refreshes while cross-shard queries stay complete. Also vendors the RedisModuleEvent_ClusterTopologyChange definitions (event id 20, subevents, info struct) into the local redismodule.h so consumers can subscribe; this mirrors redis/redis#15350 and will be reconciled by the normal redismodule.h sync. Relates-to: MOD-9152, RED-148990 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…esh on in-place reshard MR_ClusterRefreshTopology takes the change_flags reason bitmask from RedisModuleEvent_ClusterTopologyChange and picks the cheapest refresh that stays correct: - FLAG_NODE / FLAG_ROLE / FLAG_STATE (a node joined/left, a role flip, or an OK/FAIL transition -- the set of primaries may have changed) -> full MR_RefreshClusterData (reconnect to the new set of primaries). - FLAG_SLOT only (an in-place reshard: slots moved between primaries we are already connected to) -> new MR_UpdateClusterSlots, which repoints slot->node routing while reusing the existing connections, so in-flight fan-out / cross-shard queries are not aborted and slot-routed queries stay correct mid-reshard. The debounce accumulates the reason flags across the coalesced window and is race-safe: whichever refresh observes a rebuild-worthy flag does the full rebuild, so a membership/role/state change is never masked by the slot-map fast path. MR_UpdateClusterSlots self-upgrades to a full rebuild if an unknown shard appears. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the reason flags MR_UpdateClusterSlots now reconciles against a fresh CLUSTER SLOTS and rebuilds (which aborts in-flight cross-shard executions) only when the set of slot-serving primaries actually changed -- a primary entered (unknown shard) or left (distinct-count shrank). Otherwise it repoints the slot map in place, preserving connections and in-flight commands. So an over-broad or spurious topology event -- a replica re-pointing, an OK<->FAIL flip, a slotless node joining -- no longer drops in-flight multi-key commands. The debounced handler always calls this single reconcile path; the event's reason flags become advisory (change_flags is no longer consulted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review feedback (keep it as simple as possible): every topology-change event now schedules one reconcile task directly on the event loop. MR_UpdateClusterSlots already makes a redundant reconcile harmless (one CLUSTER SLOTS read, connection-preserving unless the primary set changed), so the debounce only saved reconcile churn during legacy per-slot resharding bursts, at the cost of delaying convergence by the quiet window. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The in-place reconcile matched primaries by node id only, so a primary restarting at a new address with the same id (a pod reschedule that keeps nodes.conf) kept its stale ip/port and the reconnect loop redialed the dead address forever. Compare the address from the fresh CLUSTER SLOTS reply (port via RedisModule_GetClusterNodeInfo, matching MR_RefreshClusterData) and escalate to a full rebuild on mismatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review: drop the CLUSTER SLOTS RM_Call+parse in favor of the same cluster module API the short-form CLUSTERSET builds from (GetClusterNodesList/GetClusterNodeInfo/GetClusterNodeSlotRanges), expressed as a comparable topology-view struct; the decision is a plain master-set compare (ids + addresses). Falls back to a full refresh when the API is unavailable. Trims the over-long comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A cluster-set rebuild drops all inter-shard connections and aborts every in-flight initiator execution with 'cluster topology changed'. The topology is re-broadcast on many events that do not change it (node events, shard reconnects, delivery retries), so multi-shard commands racing such a resend failed spuriously. Compare the incoming long-form command against the stored one (MYID excluded) and keep the current cluster when they match. Short-form commands still rebuild: their topology derives from the server's cluster state, not from the arguments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… skip - ClusterSetIsNewTopology replaces the double-negative ClusterSetCommandIsUnchanged - Arguments are compared with their lengths (memcmp) instead of strcmp - The short form now skips too: the derived master set is snapshotted into a canonical string (sorted by node id), stored on the cluster, and compared against the snapshot derived for the next short-form command Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both CLUSTERSET forms now feed the same reconcile the topology-change event uses (MR_TryApplyTopologyInPlace): the command arguments (long form) or the server's cluster state (short form) are reduced to a topology view and diffed against the connected shard set. An unchanged set -- including a reshard that only moves slot ranges between the same shards -- is applied in place, keeping every connection, in-flight execution and the run id; a shard entering/leaving the set, or changing its address or credentials, still tears down and rebuilds. This supersedes the short-form string-snapshot compare (removed) and extends the long-form identical-args skip: resharding no longer aborts in-flight multi-shard commands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rewritten alternation started at mock shard id '2' -- argument- identical to the topology ShardMock.__enter__ had just applied -- so the identical-args skip made iteration 0 a no-op and GetConnection() blocked forever (45-minute CI cancel, buffered stdout hid the earlier results). Start the alternation at '3' so every iteration changes the shard set relative to the last-applied command, and give GetConnection a finite timeout so a regression fails visibly instead of wedging the job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Parameterize MR_GetNode/MR_CreateNode by Cluster* instead of reaching into the clusterCtx.CurrCluster global, mirroring Gal's RedisGears#105 refactor. Keeps RedisGears#104's unified in-place reconcile + skip-identical intact; this is the connection-preserving reconcile ON the de-globalized structure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- MR_ClusterGetPassword() + FreeCluster() extraction (from RedisGears#105): removes more clusterCtx-global coupling, aids the eventual LibMR->TS lift. - MR_TopologyViewIsValid(): reject a malformed topology view (a slot claimed by >1 shard, or coverage != full slot space) before applying, keeping the current topology. Wired into the OSS-event reconcile and the long-form CLUSTERSET path. Slotless shards contribute no coverage, as expected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Consolidated test vehicle for the OSS topology-change feature: #104's unified, connection-preserving reconcile on #105's de-globalized structure.
MR_TryApplyTopologyInPlacereconcile fed by three adapters — OSS topology event, long-form CLUSTERSET, short-form CLUSTERSET — so the same mechanism serves both MOD-16382 (auto-refresh) and MOD-16399 (skip unchanged CLUSTERSET). In-place slot repoint when the master set is unchanged (connections + in-flight executions preserved); full rebuild only when a master enters/leaves or changes address/credentials.MR_GetNode/MR_CreateNodeparameterized byCluster*instead of theclusterCtx.CurrClusterglobal — makes the reconcile liftable for the eventual LibMR→TimeSeries absorption.Stacked on #100 (
MR_ClusterRefreshTopology) + #104 (CLUSTERSET unification); the diff-vs-master shows their commits until they merge — review the top de-globalization commit.Paired with RedisTimeSeries consolidated PR (module consumer + all tests from #2071 + #2088). Validated locally against merged
unstable: RTStest_asm7/7 (startup, ASM reshard, reshard-under-load, repeated cycles, short-form CLUSTERSET) +test_clusterset_noop1/1.