From 0c54e5fb83425b0bc58c29b018f4be5f35250b72 Mon Sep 17 00:00:00 2001
From: Christian Schwarz <me@cschwarz.com>
Date: Sun, 19 Jan 2025 16:21:48 +0100
Subject: [PATCH] code reading notes from last weekend

---
 docs/storage_broker.md             | 4 ++--
 safekeeper/src/timeline_manager.rs | 1 +
 safekeeper/src/wal_backup.rs       | 9 ++++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/docs/storage_broker.md b/docs/storage_broker.md
index 827fe5c5f2..c81abc1955 100644
--- a/docs/storage_broker.md
+++ b/docs/storage_broker.md
@@ -1,6 +1,6 @@
 # Storage broker
 
-Storage broker targets two issues:
+Storage broker targets two issues
 - Allowing safekeepers and pageservers learn which nodes also hold their
   timelines, and timeline statuses there.
 - Avoiding O(n^2) connections between storage nodes while doing so.
@@ -19,7 +19,7 @@ Currently, the only message is `SafekeeperTimelineInfo`. Each safekeeper, for
 each active timeline, once in a while pushes timeline status to the broker.
 Other nodes subscribe and receive this info, using it per above.
 
-Broker serves /metrics on the same port as grpc service. 
+Broker serves /metrics on the same port as grpc service.
 
 grpcurl can be used to check which values are currently being pushed:
 ```
diff --git a/safekeeper/src/timeline_manager.rs b/safekeeper/src/timeline_manager.rs
index a33994dcab..95435072d4 100644
--- a/safekeeper/src/timeline_manager.rs
+++ b/safekeeper/src/timeline_manager.rs
@@ -495,6 +495,7 @@ impl Manager {
     }
 
     /// Update is_active flag and returns its value.
+    // Timelines marked active are pushed to the broker by the `push_loop` task.
     fn update_is_active(
         &mut self,
         is_wal_backup_required: bool,
diff --git a/safekeeper/src/wal_backup.rs b/safekeeper/src/wal_backup.rs
index 8517fa0344..2c0599fe9d 100644
--- a/safekeeper/src/wal_backup.rs
+++ b/safekeeper/src/wal_backup.rs
@@ -61,7 +61,9 @@ pub(crate) fn is_wal_backup_required(
     state: &StateSnapshot,
 ) -> bool {
     num_computes > 0 ||
-    // Currently only the whole segment is offloaded, so compare segment numbers.
+    // This task backups completed segments only.
+    // The current partial segment is backed up by a separate task/code module (wal_backup_partial).
+    // So, need for completed segment backup <=> last backup was at at older segment.
     (state.commit_lsn.segment_number(wal_seg_size) > state.backup_lsn.segment_number(wal_seg_size))
 }
 
@@ -69,6 +71,11 @@ pub(crate) fn is_wal_backup_required(
 /// is me, run (per timeline) task, if not yet. OTOH, if it is not me and task
 /// is running, kill it.
 pub(crate) async fn update_task(mgr: &mut Manager, need_backup: bool, state: &StateSnapshot) {
+    // Based on the peer information received from broker, each safekeeper figures out
+    // whether it, or one of the peers, is the offloader.
+    // The algorithm is deterministic, so, if all peers have the same information,
+    // the system converges. In unconverged state, multiple peers upload the same
+    // segments, which is inefficient but safe.
     let (offloader, election_dbg_str) =
         determine_offloader(&state.peers, state.backup_lsn, mgr.tli.ttid, &mgr.conf);
     let elected_me = Some(mgr.conf.my_id) == offloader;