Reason through bootstrap.

Author: rescrv

rust/wal3/src/writer.rs

@@ -132,7 +132,17 @@ impl LogWriter {
         })
     }
 
-    /// Open or try once to initialize the log.
+    /// Given a contiguous subset of data from some other location (preferably another log),
+    /// construct a new log under storage/prefix using the provided options.
+    ///
+    /// This function is safe to run again on failure and will not bootstrap over a partially
+    /// bootstrapped collection.
+    ///
+    /// It is my intention to make this more robust as time goes on.  Concretely, that means that
+    /// as we encounter partial failures left by the tool we fix them.  There are 3 failure points
+    /// and I'd prefer to manually inspect failures than get the automation right to do it always
+    /// automatically.  Bootstrap is intended only to last as long as there is a migration from the
+    /// go to the rust log services.
     pub async fn bootstrap<D: MarkDirty>(
         options: &LogWriterOptions,
         storage: &Arc<Storage>,
@@ -141,14 +151,23 @@ impl LogWriter {
         mark_dirty: D,
         first_record_offset: LogPosition,
         messages: Vec<Vec<u8>>,
-    ) -> Result<Self, Error> {
+    ) -> Result<(), Error> {
         let num_records = messages.len();
         let start = first_record_offset;
         let limit = first_record_offset + num_records;
+        // SAFETY(rescrv):  This is a speculative load to narrow the window in which we would see a
+        // race between writers.
         let manifest = Manifest::load(&ThrottleOptions::default(), storage, prefix).await?;
         if manifest.is_some() {
             return Err(Error::LogContention);
         }
+        // SAFETY(rescrv):  This will only succeed if the file doesn't exist.  Technically the log
+        // could be initialized and garbage collected to leave a prefix hole, but our timing
+        // assumption is that every op happens in less than 1/2 the GC interval, so there's no way
+        // for that to happen.
+        //
+        // If the file exists, this will fail with LogContention, which fails us with
+        // LogContention.  Other errors fail transparently, too.
         let (unprefixed_path, setsum, num_bytes) = upload_parquet(
             options,
             storage,
@@ -158,7 +177,9 @@ impl LogWriter {
             messages,
         )
         .await?;
+        // SAFETY(rescrv):  Any error here is an error.
         Manifest::initialize(options, storage, prefix, writer).await?;
+        // SAFETY(rescrv):  We just initialized, so we should be able to load---done to get e_tag.
         let Some((manifest, e_tag)) =
             Manifest::load(&ThrottleOptions::default(), storage, prefix).await?
         else {
@@ -177,11 +198,13 @@ impl LogWriter {
             setsum,
         };
         let mut new_manifest = manifest.clone();
+        // SAFETY(rescrv):  This is unit tested to never happen.  If it happens, add more tests.
         if !new_manifest.can_apply_fragment(&frag) {
             tracing::error!("Cannot apply frag to a clean manifest.");
             return Err(Error::Internal);
         }
         new_manifest.apply_fragment(frag);
+        // SAFETY(rescrv):  If this fails, there's nothing left to do.
         manifest
             .install(
                 &ThrottleOptions::default(),
@@ -191,8 +214,11 @@ impl LogWriter {
                 &new_manifest,
             )
             .await?;
+        // Not Safety:
+        // We mark dirty, but if we lose that we lose that.
+        // Failure to mark dirty fails the bootstrap.
         mark_dirty.mark_dirty(limit, num_records).await?;
-        todo!();
+        Ok(())
     }
 
     /// This will close the log.
@@ -563,6 +589,9 @@ pub async fn upload_parquet(
     loop {
         let (buffer, setsum) = construct_parquet(log_position, &messages)?;
         tracing::info!("upload_parquet: {:?} with {} bytes", path, buffer.len());
+        // NOTE(rescrv):  This match block has been thoroughly reasoned through within the
+        // `bootstrap` call above.  Don't change the error handling here without re-reasoning
+        // there.
         match storage
             .put_bytes(
                 &path,