From def9b17b1502e01ce51182a7bff12595928d38b6 Mon Sep 17 00:00:00 2001 From: Vlad Lazar Date: Thu, 18 Apr 2024 15:52:28 +0100 Subject: [PATCH] pageserver: achieve parity between read paths for invalid lsns --- pageserver/src/tenant/timeline.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index e9892a0002..dc31cd5fb2 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -590,8 +590,8 @@ impl From for PageReconstructError { fn from(e: GetVectoredError) -> Self { match e { GetVectoredError::Cancelled => PageReconstructError::Cancelled, + GetVectoredError::InvalidLsn(_) => PageReconstructError::Other(anyhow!("Invalid LSN")), err @ GetVectoredError::Oversized(_) => PageReconstructError::Other(err.into()), - err @ GetVectoredError::InvalidLsn(_) => PageReconstructError::Other(err.into()), err @ GetVectoredError::MissingKey(_) => PageReconstructError::Other(err.into()), err @ GetVectoredError::GetReadyAncestorError(_) => PageReconstructError::from(err), GetVectoredError::Other(err) => PageReconstructError::Other(err), @@ -707,6 +707,15 @@ impl Timeline { lsn: Lsn, ctx: &RequestContext, ) -> Result { + if !lsn.is_valid() { + return Err(PageReconstructError::Other(anyhow::anyhow!("Invalid LSN"))); + } + + // This check is debug-only because of the cost of hashing, and because it's a double-check: we + // already checked the key against the shard_identity when looking up the Timeline from + // page_service. + debug_assert!(!self.shard_identity.is_key_disposable(&key)); + self.timeline_get_throttle.throttle(ctx, 1).await; // Check the page cache. We will get back the most recent page with lsn <= `lsn`. @@ -798,15 +807,6 @@ impl Timeline { mut reconstruct_state: ValueReconstructState, ctx: &RequestContext, ) -> Result { - if !lsn.is_valid() { - return Err(PageReconstructError::Other(anyhow::anyhow!("Invalid LSN"))); - } - - // This check is debug-only because of the cost of hashing, and because it's a double-check: we - // already checked the key against the shard_identity when looking up the Timeline from - // page_service. - debug_assert!(!self.shard_identity.is_key_disposable(&key)); - // XXX: structured stats collection for layer eviction here. trace!( "get page request for {}@{} from task kind {:?}",