raft: minor tweaks

Author: erikgrinaker

src/error.rs

@@ -42,7 +42,7 @@ impl Error {
     /// machine application needs to know whether a command failure is
     /// deterministic on the input command -- if it is, the command can be
     /// considered applied and the error returned to the client, but otherwise
-    /// the state machine must panic to prevent replica divergence.
+    /// the state machine must panic to prevent node divergence.
     pub fn is_deterministic(&self) -> bool {
         match self {
             // Aborts don't happen during application, only leader changes. But

src/raft/log.rs

@@ -7,13 +7,16 @@ use crate::encoding::{self, Key as _, Value as _, bincode};
 use crate::error::Result;
 use crate::storage;
 
-/// A log index. Starts at 1, indicates no index if 0.
+/// A log index (entry position). Starts at 1. 0 indicates no index.
 pub type Index = u64;
 
-/// A log entry.
+/// A log entry containing a state machine command.
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub struct Entry {
     /// The entry index.
+    ///
+    /// We could omit the index in the encoded value, since it's also stored in
+    /// the key, but we keep it simple.
     pub index: Index,
     /// The term in which the entry was added.
     pub term: Term,
@@ -106,15 +109,16 @@ pub struct Log {
     /// If true, fsync entries to disk when appended. This is mandated by Raft,
     /// but comes with a hefty performance penalty (especially since we don't
     /// optimize for it by batching entries before fsyncing). Disabling it will
-    /// yield much better write performance, but may lose data on host crashes,
-    /// which in some scenarios can cause log entries to become "uncommitted"
-    /// and state machines diverging.
+    /// yield much better write performance, but may lose data on crashes, which
+    /// in some scenarios can cause log entries to become "uncommitted" and
+    /// state machines diverging.
     fsync: bool,
 }
 
 impl Log {
     /// Initializes a log using the given storage engine.
     pub fn new(mut engine: Box<dyn storage::Engine>) -> Result<Self> {
+        // Load some initial in-memory state from disk.
         let (term, vote) = engine
             .get(&Key::TermVote.encode())?
             .map(|v| bincode::deserialize(&v))
@@ -136,7 +140,8 @@ impl Log {
             .map(|v| bincode::deserialize(&v))
             .transpose()?
             .unwrap_or((0, 0));
-        let fsync = true; // fsync by default (NB: BitCask::flush() is a noop in tests)
+
+        let fsync = true; // fsync by default
         Ok(Self { engine, term, vote, last_index, last_term, commit_index, commit_term, fsync })
     }
 
@@ -168,11 +173,12 @@ impl Log {
         assert!(term > 0, "can't set term 0");
         assert!(term >= self.term, "term regression {} → {}", self.term, term);
         assert!(term > self.term || self.vote.is_none() || vote == self.vote, "can't change vote");
+
         if term == self.term && vote == self.vote {
             return Ok(());
         }
         self.engine.set(&Key::TermVote.encode(), bincode::serialize(&(term, vote)))?;
-        // Always fsync, even with Log.fsync = false. Term changes are rare, so
+        // Always fsync, even with Log::fsync = false. Term changes are rare, so
         // this doesn't materially affect performance, and double voting could
         // lead to multiple leaders and split brain which is really bad.
         self.engine.flush()?;
@@ -186,8 +192,6 @@ impl Log {
     /// Raft leader changes.
     pub fn append(&mut self, command: Option<Vec<u8>>) -> Result<Index> {
         assert!(self.term > 0, "can't append entry in term 0");
-        // We could omit the index in the encoded value, since it's also stored
-        // in the key, but we keep it simple.
         let entry = Entry { index: self.last_index + 1, term: self.term, command };
         self.engine.set(&Key::Entry(entry.index).encode(), entry.encode())?;
         if self.fsync {
@@ -202,16 +206,16 @@ impl Log {
     /// exist and be at or after the current commit index.
     pub fn commit(&mut self, index: Index) -> Result<Index> {
         let term = match self.get(index)? {
-            Some(e) if e.index < self.commit_index => {
-                panic!("commit index regression {} → {}", self.commit_index, e.index);
+            Some(entry) if entry.index < self.commit_index => {
+                panic!("commit index regression {} → {}", self.commit_index, entry.index);
             }
-            Some(e) if e.index == self.commit_index => return Ok(index),
-            Some(e) => e.term,
+            Some(entry) if entry.index == self.commit_index => return Ok(index),
+            Some(entry) => entry.term,
             None => panic!("commit index {index} does not exist"),
         };
         self.engine.set(&Key::CommitIndex.encode(), bincode::serialize(&(index, term)))?;
         // NB: the commit index doesn't need to be fsynced, since the entries
-        // are fsynced and the commit index can be recovered from a log quorum.
+        // are fsynced and the commit index can be recovered from the quorum.
         self.commit_index = index;
         self.commit_term = term;
         Ok(index)
@@ -255,21 +259,22 @@ impl Log {
     pub fn scan_apply(&mut self, applied_index: Index) -> Iterator {
         // NB: we don't assert that commit_index >= applied_index, because the
         // local commit index is not flushed to durable storage -- if lost on
-        // restart, it can be recovered from a quorum of logs.
+        // restart, it can be recovered from the logs of a quorum.
         if applied_index >= self.commit_index {
             return Iterator::new(Box::new(std::iter::empty()));
         }
         self.scan(applied_index + 1..=self.commit_index)
     }
 
-    /// Splices a set of entries into the log and flushes it to disk. The
-    /// entries must have contiguous indexes and equal/increasing terms, and the
-    /// first entry must be in the range [1,last_index+1] with a term at or
+    /// Splices a set of entries into the log and flushes it to disk.  New
+    /// indexes will be appended. Overlapping indexes with the same term must be
+    /// equal and will be ignored. Overlapping indexes with different terms will
+    /// truncate the existing log at the first conflict and then splice the new
+    /// entries.
+    ///
+    /// The entries must have contiguous indexes and equal/increasing terms, and
+    /// the first entry must be in the range [1,last_index+1] with a term at or
     /// above the previous (base) entry's term and at or below the current term.
-    /// New indexes will be appended. Overlapping indexes with the same term
-    /// must be equal and will be ignored. Overlapping indexes with different
-    /// terms will truncate the existing log at the first conflict and then
-    /// splice the new entries.
     pub fn splice(&mut self, entries: Vec<Entry>) -> Result<Index> {
         let (Some(first), Some(last)) = (entries.first(), entries.last()) else {
             return Ok(self.last_index); // empty input is noop

src/raft/message.rs

@@ -25,10 +25,10 @@ impl encoding::Value for Envelope {}
 /// A message sent between Raft nodes. Messages are sent asynchronously (i.e.
 /// they are not request/response) and may be dropped or reordered.
 ///
-/// In practice, they are sent across a TCP connection and crossbeam channels
-/// ensuring messages are not dropped or reordered as long as the connection
-/// remains intact. A message and its response are sent across separate TCP
-/// connections (outbound from their respective senders).
+/// In practice, they are sent across a TCP connection and crossbeam channel
+/// which ensures messages are not dropped or reordered as long as the
+/// connection remains intact. A message and its response are sent across
+/// separate TCP connections (outbound from the respective sender).
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub enum Message {
     /// Candidates campaign for leadership by soliciting votes from peers.
@@ -151,10 +151,11 @@ pub enum Message {
     },
 }
 
-/// A client request ID. Must be globally unique for the duration of the
-/// request. For simplicity, a random UUIDv4 is used -- the node ID and process
-/// ID could be incorporated for further collision avoidance, but it does not
-/// matter at this scale.
+/// A client request ID. Must be globally unique while in flight.
+///
+/// For simplicity, a random UUIDv4 is used. We could incorporate the
+/// node/process/MAC ID and timestamp for better collision avoidance (e.g. via
+/// UUIDv6) but it doesn't matter at this scale.
 pub type RequestID = uuid::Uuid;
 
 /// A read sequence number, used to confirm leadership for linearizable reads.

src/raft/mod.rs

@@ -188,7 +188,7 @@
 //! machine, the leader looks up the write request by its log index and sends
 //! the result to the client. Deterministic errors (e.g. foreign key violations)
 //! are also returned to the client, but non-deterministic errors (e.g. IO
-//! errors) must panic the node to avoid replica state divergence.
+//! errors) must panic the node to avoid state divergence.
 //!
 //! Read requests, `Request::Read`, are only executed on the leader and don't
 //! need to be replicated via the Raft log. However, to ensure linearizability,
@@ -252,15 +252,15 @@ pub use message::{Envelope, Message, ReadSequence, Request, RequestID, Response,
 pub use node::{Node, NodeID, Options, Term, Ticks};
 pub use state::State;
 
-/// The interval between Raft ticks, the unit of time.
+/// The interval between Raft ticks, the Raft unit of time.
 pub const TICK_INTERVAL: Duration = Duration::from_millis(100);
 
 /// The interval between leader heartbeats in ticks.
 const HEARTBEAT_INTERVAL: Ticks = 4;
 
-/// The default election timeout range in ticks. This is randomized in this
-/// interval, to avoid election ties.
+/// The default election timeout range in ticks. To avoid election ties, a node
+/// chooses a random value in this interval.
 const ELECTION_TIMEOUT_RANGE: Range<Ticks> = 10..20;
 
-/// The maximum number of entries to send in a single append message.
+/// The maximum number of log entries to send in a single append message.
 const MAX_APPEND_ENTRIES: usize = 100;

src/raft/node.rs

@@ -522,7 +522,7 @@ impl RawNode<Follower> {
             debug!("Applying {entry:?}");
             // Throw away the result, since only the leader responds to clients.
             // This includes errors -- any non-deterministic errors (e.g. IO
-            // errors) must panic instead to avoid replica divergence.
+            // errors) must panic instead to avoid node divergence.
             _ = self.state.apply(entry);
         }
         Ok(())

src/raft/state.rs

@@ -6,18 +6,18 @@ use crate::error::Result;
 /// arbitrary binary commands sequentially from the Raft log, returning an
 /// arbitrary binary result to the client.
 ///
-/// Since commands are applied identically across all replicas, they must be
-/// deterministic and yield the same state and result across all replicas too.
-/// Otherwise, the replicas will diverge, and different replicas will produce
+/// Since commands are applied identically across all nodes, they must be
+/// deterministic and yield the same state and result across all nodes too.
+/// Otherwise, the nodes will diverge, such that different nodes will produce
 /// different results.
 ///
-/// Write commands (`Request::Write`) are replicated and applied on all replicas
+/// Write commands (`Request::Write`) are replicated and applied on all nodes
 /// via `State::apply`. The state machine must keep track of the last applied
 /// index and return it via `State::get_applied_index`. Read commands
-/// (`Request::Read`) are only executed on a single replica via `State::read`
-/// and must not make any state changes.
+/// (`Request::Read`) are only executed on a single node via `State::read` and
+/// must not make any state changes.
 pub trait State: Send {
-    /// Returns the last applied index from the state machine.
+    /// Returns the last applied log index from the state machine.
     ///
     /// This must correspond to the current state of the state machine, since it
     /// determines which command to apply next. In particular, a node crash may
@@ -28,13 +28,13 @@ pub trait State: Send {
     /// Applies a log entry to the state machine, returning a client result.
     /// Errors are considered applied and propagated back to the client.
     ///
-    /// This is executed on all replicas, so the result must be deterministic:
-    /// it must yield the same state and result on all replicas, even if the
-    /// command is reapplied following a node crash.
+    /// This is executed on all nodes, so the result must be deterministic: it
+    /// must yield the same state and result on all nodes, even if the command
+    /// is reapplied following a node crash.
     ///
     /// Any non-deterministic apply error (e.g. an IO error) must panic and
     /// crash the node -- if it instead returns an error to the client, the
-    /// command is considered applied and replica states will diverge. The state
+    /// command is considered applied and node states will diverge. The state
     /// machine is responsible for panicing when appropriate.
     ///
     /// The entry may contain a noop command, which is committed by Raft during
@@ -45,8 +45,8 @@ pub trait State: Send {
     /// Executes a read command in the state machine, returning a client result.
     /// Errors are also propagated back to the client.
     ///
-    /// This is only executed on a single replica/node, so it must not result in
-    /// any state changes (i.e. it must not write).
+    /// This is only executed on a single node, so it must not result in any
+    /// state changes (i.e. it must not write).
     fn read(&self, command: Vec<u8>) -> Result<Vec<u8>>;
 }
 

src/sql/engine/raft.rs

@@ -282,8 +282,8 @@ impl<E: storage::Engine> raft::State for State<E> {
 
         let result = match &entry.command {
             Some(command) => match self.write(Write::decode(command)?) {
-                // Panic on non-deterministic apply failures, to prevent replica
-                // divergence. See [`raft::State`] docs for details.
+                // Panic on non-deterministic apply failures, to prevent node
+                // state divergence. See [`raft::State`] docs for details.
                 Err(e) if !e.is_deterministic() => panic!("non-deterministic apply failure: {e}"),
                 result => result,
             },