From 3454ef7507000f05707418324dad392b366f8de3 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 18 Sep 2024 13:16:51 +0300 Subject: [PATCH] Refactor ImageLayerWriter to avoid passing a Timeline to finish() (#9028) Commit ca5390a89d made a similar change to DeltaLayerWriter. We bumped into this with Stas with our hackathon project, to create a standalong program to create image layers directly from a Postgres data directory. It needs to create image layers without having a Timeline and other pageserver machinery. This downgrades the "created image layer {}" message from INFO to TRACE level. TRACE is used for the corresponding message on delta layer creation too. The path logged in the message is now the temporary path, before the file is renamed to its final name. Again commit ca5390a89d made the same change for the message on delta layer creation. --- .../src/tenant/storage_layer/image_layer.rs | 43 ++++++++----------- .../src/tenant/storage_layer/split_writer.rs | 16 +++---- pageserver/src/tenant/timeline.rs | 9 ++-- pageserver/src/tenant/timeline/compaction.rs | 6 ++- 4 files changed, 35 insertions(+), 39 deletions(-) diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 875e223c9c..5de2582ab7 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -38,7 +38,7 @@ use crate::tenant::timeline::GetVectoredError; use crate::tenant::vectored_blob_io::{ BlobFlag, StreamingVectoredReadPlanner, VectoredBlobReader, VectoredRead, VectoredReadPlanner, }; -use crate::tenant::{PageReconstructError, Timeline}; +use crate::tenant::PageReconstructError; use crate::virtual_file::owned_buffers_io::io_buf_ext::IoBufExt; use crate::virtual_file::{self, VirtualFile}; use crate::{IMAGE_FILE_MAGIC, STORAGE_FORMAT_VERSION, TEMP_FILE_SUFFIX}; @@ -58,7 +58,6 @@ use std::io::SeekFrom; use std::ops::Range; use std::os::unix::prelude::FileExt; use std::str::FromStr; -use std::sync::Arc; use tokio::sync::OnceCell; use tokio_stream::StreamExt; use tracing::*; @@ -70,9 +69,7 @@ use utils::{ }; use super::layer_name::ImageLayerName; -use super::{ - AsLayerDesc, Layer, LayerName, PersistentLayerDesc, ResidentLayer, ValuesReconstructState, -}; +use super::{AsLayerDesc, LayerName, PersistentLayerDesc, ValuesReconstructState}; /// /// Header stored in the beginning of the file @@ -800,10 +797,9 @@ impl ImageLayerWriterInner { /// async fn finish( self, - timeline: &Arc, ctx: &RequestContext, end_key: Option, - ) -> anyhow::Result { + ) -> anyhow::Result<(PersistentLayerDesc, Utf8PathBuf)> { let index_start_blk = ((self.blob_writer.size() + PAGE_SZ as u64 - 1) / PAGE_SZ as u64) as u32; @@ -879,12 +875,9 @@ impl ImageLayerWriterInner { // fsync the file file.sync_all().await?; - // FIXME: why not carry the virtualfile here, it supports renaming? - let layer = Layer::finish_creating(self.conf, timeline, desc, &self.path)?; + trace!("created image layer {}", self.path); - info!("created image layer {}", layer.local_path()); - - Ok(layer) + Ok((desc, self.path)) } } @@ -963,24 +956,18 @@ impl ImageLayerWriter { /// pub(crate) async fn finish( mut self, - timeline: &Arc, ctx: &RequestContext, - ) -> anyhow::Result { - self.inner.take().unwrap().finish(timeline, ctx, None).await + ) -> anyhow::Result<(PersistentLayerDesc, Utf8PathBuf)> { + self.inner.take().unwrap().finish(ctx, None).await } /// Finish writing the image layer with an end key, used in [`super::split_writer::SplitImageLayerWriter`]. The end key determines the end of the image layer's covered range and is exclusive. pub(super) async fn finish_with_end_key( mut self, - timeline: &Arc, end_key: Key, ctx: &RequestContext, - ) -> anyhow::Result { - self.inner - .take() - .unwrap() - .finish(timeline, ctx, Some(end_key)) - .await + ) -> anyhow::Result<(PersistentLayerDesc, Utf8PathBuf)> { + self.inner.take().unwrap().finish(ctx, Some(end_key)).await } } @@ -1084,7 +1071,7 @@ mod test { tenant::{ config::TenantConf, harness::{TenantHarness, TIMELINE_ID}, - storage_layer::ResidentLayer, + storage_layer::{Layer, ResidentLayer}, vectored_blob_io::StreamingVectoredReadPlanner, Tenant, Timeline, }, @@ -1155,7 +1142,8 @@ mod test { key = key.next(); } - writer.finish(&timeline, &ctx).await.unwrap() + let (desc, path) = writer.finish(&ctx).await.unwrap(); + Layer::finish_creating(tenant.conf, &timeline, desc, &path).unwrap() }; let original_size = resident.metadata().file_size; @@ -1217,7 +1205,9 @@ mod test { .await .unwrap(); let replacement = if wrote_keys > 0 { - Some(filtered_writer.finish(&timeline, &ctx).await.unwrap()) + let (desc, path) = filtered_writer.finish(&ctx).await.unwrap(); + let resident = Layer::finish_creating(tenant.conf, &timeline, desc, &path).unwrap(); + Some(resident) } else { None }; @@ -1290,7 +1280,8 @@ mod test { for (key, img) in images { writer.put_image(key, img, ctx).await?; } - let img_layer = writer.finish(tline, ctx).await?; + let (desc, path) = writer.finish(ctx).await?; + let img_layer = Layer::finish_creating(tenant.conf, tline, desc, &path)?; Ok::<_, anyhow::Error>(img_layer) } diff --git a/pageserver/src/tenant/storage_layer/split_writer.rs b/pageserver/src/tenant/storage_layer/split_writer.rs index 40a6a77a50..b499a0eef4 100644 --- a/pageserver/src/tenant/storage_layer/split_writer.rs +++ b/pageserver/src/tenant/storage_layer/split_writer.rs @@ -121,11 +121,11 @@ impl SplitImageLayerWriter { self.generated_layers .push(SplitWriterResult::Discarded(layer_key)); } else { - self.generated_layers.push(SplitWriterResult::Produced( - prev_image_writer - .finish_with_end_key(tline, key, ctx) - .await?, - )); + let (desc, path) = prev_image_writer.finish_with_end_key(key, ctx).await?; + + let layer = Layer::finish_creating(self.conf, tline, desc, &path)?; + self.generated_layers + .push(SplitWriterResult::Produced(layer)); } } self.inner.put_image(key, img, ctx).await @@ -170,9 +170,9 @@ impl SplitImageLayerWriter { if discard(&layer_key).await { generated_layers.push(SplitWriterResult::Discarded(layer_key)); } else { - generated_layers.push(SplitWriterResult::Produced( - inner.finish_with_end_key(tline, end_key, ctx).await?, - )); + let (desc, path) = inner.finish_with_end_key(end_key, ctx).await?; + let layer = Layer::finish_creating(self.conf, tline, desc, &path)?; + generated_layers.push(SplitWriterResult::Produced(layer)); } Ok(generated_layers) } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 262dccac7d..f66491d962 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -4013,7 +4013,8 @@ impl Timeline { if wrote_keys { // Normal path: we have written some data into the new image layer for this // partition, so flush it to disk. - let image_layer = image_layer_writer.finish(self, ctx).await?; + let (desc, path) = image_layer_writer.finish(ctx).await?; + let image_layer = Layer::finish_creating(self.conf, self, desc, &path)?; Ok(ImageLayerCreationOutcome { image: Some(image_layer), next_start_key: img_range.end, @@ -4101,7 +4102,8 @@ impl Timeline { if wrote_any_image { // Normal path: we have written some data into the new image layer for this // partition, so flush it to disk. - let image_layer = image_layer_writer.finish(self, ctx).await?; + let (desc, path) = image_layer_writer.finish(ctx).await?; + let image_layer = Layer::finish_creating(self.conf, self, desc, &path)?; Ok(ImageLayerCreationOutcome { image: Some(image_layer), next_start_key: img_range.end, @@ -5403,7 +5405,8 @@ impl Timeline { for (key, img) in images { image_layer_writer.put_image(key, img, ctx).await?; } - let image_layer = image_layer_writer.finish(self, ctx).await?; + let (desc, path) = image_layer_writer.finish(ctx).await?; + let image_layer = Layer::finish_creating(self.conf, self, desc, &path)?; { let mut guard = self.layers.write().await; diff --git a/pageserver/src/tenant/timeline/compaction.rs b/pageserver/src/tenant/timeline/compaction.rs index 0b5c520ba7..d1f06e3480 100644 --- a/pageserver/src/tenant/timeline/compaction.rs +++ b/pageserver/src/tenant/timeline/compaction.rs @@ -563,10 +563,12 @@ impl Timeline { .await?; if keys_written > 0 { - let new_layer = image_layer_writer - .finish(self, ctx) + let (desc, path) = image_layer_writer + .finish(ctx) .await .map_err(CompactionError::Other)?; + let new_layer = Layer::finish_creating(self.conf, self, desc, &path) + .map_err(CompactionError::Other)?; tracing::info!(layer=%new_layer, "Rewrote layer, {} -> {} bytes", layer.metadata().file_size, new_layer.metadata().file_size);