refactor(cluster): extract emit helper, add NodeId::as_key, clarify format_flags - #166
Merged
Conversation
…ormat_flags gossip.rs - extract `async fn emit(&self, event: GossipEvent)` that sends on event_tx and warns when the channel is closed. replaces 6 copies of the inline send-and-check pattern in apply_updates and mark_alive. MemberJoined and MemberSuspected were previously fire-and-forget (let _ = ...); they now consistently warn on channel closure like the other events. - add comment to queue_update explaining why dropping the oldest pending updates is safe (convergence doesn't require every update to be delivered). topology.rs - add NodeId::as_key() returning the full UUID string for use as a map key. NodeId::Display intentionally shows only 8 chars for readability, which is too short to use as a unique key in storage. - add doc comment to format_flags explaining the two intentional divergences from NodeFlags::Display: role is included, and pfail renders as "fail?" per the Redis cluster protocol spec rather than "pfail". raft.rs - replace node_id.0.to_string() with node_id.as_key() throughout apply_command. the UUID string access is now named rather than relying on tuple field indexing.
kacy
added a commit
that referenced
this pull request
Feb 19, 2026
…ormat_flags (#166) gossip.rs - extract `async fn emit(&self, event: GossipEvent)` that sends on event_tx and warns when the channel is closed. replaces 6 copies of the inline send-and-check pattern in apply_updates and mark_alive. MemberJoined and MemberSuspected were previously fire-and-forget (let _ = ...); they now consistently warn on channel closure like the other events. - add comment to queue_update explaining why dropping the oldest pending updates is safe (convergence doesn't require every update to be delivered). topology.rs - add NodeId::as_key() returning the full UUID string for use as a map key. NodeId::Display intentionally shows only 8 chars for readability, which is too short to use as a unique key in storage. - add doc comment to format_flags explaining the two intentional divergences from NodeFlags::Display: role is included, and pfail renders as "fail?" per the Redis cluster protocol spec rather than "pfail". raft.rs - replace node_id.0.to_string() with node_id.as_key() throughout apply_command. the UUID string access is now named rather than relying on tuple field indexing.
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.
summary
three targeted changes across ember-cluster to eliminate repeated patterns and clarify non-obvious design decisions.
gossip.rs—emithelperevent_tx.send(...).awaitwith error handling appeared 6 times inapply_updatesandmark_alive. The patterns were inconsistent: MemberAlive, MemberFailed, and MemberLeft logged warnings on channel close, but MemberJoined and MemberSuspected were fire-and-forget (let _ = ...). This inconsistency had no intentional basis.Extracted
async fn emit(&self, event: GossipEvent)that sends and warns when the channel is closed (which only happens during shutdown or if the event consumer has a bug). All 6 call sites now behave consistently. Also added a comment onqueue_updateexplaining why dropping oldest updates on overflow is safe: gossip convergence doesn't require every update to be delivered — members re-gossip state on each protocol period.topology.rs+raft.rs—NodeId::as_key()node_id.0.to_string()appeared 7 times inapply_command(raft.rs). This accesses the inner UUID by tuple field index, which is opaque and could be confused withNodeId::to_string()(which only shows 8 chars viaDisplay). Addedpub(crate) fn as_key(&self) -> StringonNodeIdthat makes the intent explicit — full UUID for use as a storage key — and replaced all 7 call sites.topology.rs—format_flagscommentClusterNode::format_flags()andNodeFlags::Displayoverlap but differ in two ways: role (master/slave) is included informat_flagsbut is not a flag, andpfailrenders asfail?informat_flagsper the Redis cluster protocol spec. Without a comment, these look like they should be deduplicated but can't be without breaking the wire format.what was tested
cargo build -p ember-cluster— cleancargo test -p ember-cluster— 63 tests pass, 0 failurescargo fmt --check -p ember-cluster— no formatting issues