mirror of
https://github.com/lancedb/lancedb.git
synced 2026-08-18 12:08:35 +00:00
fix(rust): handle missing mirrored copy sources (#3843)
## Summary - treat `NotFound` from the mirrored secondary copy as a cache miss while preserving every other secondary error - perform the durable primary copy after either a successful secondary copy or a secondary cache miss - cover both an initially missing secondary manifest and eviction immediately before the secondary copy ## Root cause Readers can use process-local secondary stores that do not contain a staging manifest written by another process, or that evict it before finalization. `MirroringObjectStore::copy_opts` propagated that secondary `NotFound`, so older object_store versions could loop indefinitely and the locked version aborted before performing the durable primary copy. ## Validation - `cargo fmt --all -- --check` - `cargo test --quiet --features remote -p lancedb io::object_store::test::test_copy_when -- --nocapture` - `cargo check --quiet --features remote --tests --examples` - `cargo clippy --quiet --features remote --tests --examples` - `cargo test --quiet --features remote --tests` Fixes #1176 <!-- lance-gatekeeper-fix:v1 agent=636210af9dcd25b6dceadebd2fcafc6f generation=1 --> --------- Co-authored-by: Gatefixer <313497061+lancedb-gatefixer[bot]@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
cc0139c136
commit
e6444ecc05
@@ -132,9 +132,14 @@ impl ObjectStore for MirroringObjectStore {
|
||||
if to.primary_only() {
|
||||
self.primary.copy_opts(from, to, options).await
|
||||
} else {
|
||||
self.secondary.copy_opts(from, to, options.clone()).await?;
|
||||
self.primary.copy_opts(from, to, options).await?;
|
||||
Ok(())
|
||||
// The secondary store can be process-local and less durable than the
|
||||
// primary, so a source written by another process may not exist here
|
||||
// or may be evicted before the copy begins.
|
||||
match self.secondary.copy_opts(from, to, options.clone()).await {
|
||||
Ok(()) | Err(Error::NotFound { .. }) => {}
|
||||
Err(err) => return Err(err),
|
||||
}
|
||||
self.primary.copy_opts(from, to, options).await
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -192,7 +197,8 @@ mod test {
|
||||
use futures::TryStreamExt;
|
||||
use lance::{dataset::WriteParams, io::ObjectStoreParams};
|
||||
use lance_testing::datagen::{BatchGenerator, IncrementingInt32, RandomVector};
|
||||
use object_store::local::LocalFileSystem;
|
||||
use object_store::{local::LocalFileSystem, memory::InMemory};
|
||||
use std::time::Duration;
|
||||
use tempfile;
|
||||
|
||||
use crate::{
|
||||
@@ -201,6 +207,139 @@ mod test {
|
||||
table::WriteOptions,
|
||||
};
|
||||
|
||||
#[derive(Debug)]
|
||||
struct EvictBeforeCopyStore {
|
||||
inner: Arc<dyn ObjectStore>,
|
||||
}
|
||||
|
||||
impl std::fmt::Display for EvictBeforeCopyStore {
|
||||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
|
||||
write!(f, "EvictBeforeCopyStore")
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl ObjectStore for EvictBeforeCopyStore {
|
||||
async fn put_opts(
|
||||
&self,
|
||||
location: &Path,
|
||||
payload: PutPayload,
|
||||
options: PutOptions,
|
||||
) -> Result<PutResult> {
|
||||
self.inner.put_opts(location, payload, options).await
|
||||
}
|
||||
|
||||
async fn put_multipart_opts(
|
||||
&self,
|
||||
location: &Path,
|
||||
options: PutMultipartOptions,
|
||||
) -> Result<Box<dyn MultipartUpload>> {
|
||||
self.inner.put_multipart_opts(location, options).await
|
||||
}
|
||||
|
||||
async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult> {
|
||||
self.inner.get_opts(location, options).await
|
||||
}
|
||||
|
||||
fn delete_stream(
|
||||
&self,
|
||||
locations: BoxStream<'static, Result<Path>>,
|
||||
) -> BoxStream<'static, Result<Path>> {
|
||||
self.inner.delete_stream(locations)
|
||||
}
|
||||
|
||||
fn list(&self, prefix: Option<&Path>) -> BoxStream<'static, Result<ObjectMeta>> {
|
||||
self.inner.list(prefix)
|
||||
}
|
||||
|
||||
async fn list_with_delimiter(&self, prefix: Option<&Path>) -> Result<ListResult> {
|
||||
self.inner.list_with_delimiter(prefix).await
|
||||
}
|
||||
|
||||
async fn copy_opts(&self, from: &Path, to: &Path, options: CopyOptions) -> Result<()> {
|
||||
self.inner.delete(from).await?;
|
||||
self.inner.copy_opts(from, to, options).await
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_copy_when_source_is_missing_from_secondary() {
|
||||
let primary_dir = tempfile::tempdir().unwrap();
|
||||
let secondary_dir = tempfile::tempdir().unwrap();
|
||||
let primary: Arc<dyn ObjectStore> =
|
||||
Arc::new(LocalFileSystem::new_with_prefix(primary_dir.path()).unwrap());
|
||||
let secondary: Arc<dyn ObjectStore> =
|
||||
Arc::new(LocalFileSystem::new_with_prefix(secondary_dir.path()).unwrap());
|
||||
let store = MirroringObjectStore {
|
||||
primary: primary.clone(),
|
||||
secondary: secondary.clone(),
|
||||
};
|
||||
let staging = Path::from("_versions/1.manifest-staging");
|
||||
let finalized = Path::from("_versions/1.manifest");
|
||||
|
||||
primary
|
||||
.put(&staging, "manifest contents".into())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
tokio::time::timeout(Duration::from_secs(5), store.copy(&staging, &finalized))
|
||||
.await
|
||||
.expect("copy should not hang when the secondary source is missing")
|
||||
.unwrap();
|
||||
|
||||
let copied = primary
|
||||
.get(&finalized)
|
||||
.await
|
||||
.unwrap()
|
||||
.bytes()
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(copied, "manifest contents");
|
||||
assert!(matches!(
|
||||
secondary.head(&finalized).await,
|
||||
Err(Error::NotFound { .. })
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_copy_when_secondary_source_disappears_after_head() {
|
||||
let primary: Arc<dyn ObjectStore> = Arc::new(InMemory::new());
|
||||
let secondary_inner: Arc<dyn ObjectStore> = Arc::new(InMemory::new());
|
||||
let secondary: Arc<dyn ObjectStore> = Arc::new(EvictBeforeCopyStore {
|
||||
inner: secondary_inner.clone(),
|
||||
});
|
||||
let store = MirroringObjectStore {
|
||||
primary: primary.clone(),
|
||||
secondary,
|
||||
};
|
||||
let staging = Path::from("_versions/1.manifest-staging");
|
||||
let finalized = Path::from("_versions/1.manifest");
|
||||
|
||||
primary
|
||||
.put(&staging, "manifest contents".into())
|
||||
.await
|
||||
.unwrap();
|
||||
secondary_inner
|
||||
.put(&staging, "manifest contents".into())
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
store.copy(&staging, &finalized).await.unwrap();
|
||||
|
||||
let copied = primary
|
||||
.get(&finalized)
|
||||
.await
|
||||
.unwrap()
|
||||
.bytes()
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(copied, "manifest contents");
|
||||
assert!(matches!(
|
||||
secondary_inner.head(&finalized).await,
|
||||
Err(Error::NotFound { .. })
|
||||
));
|
||||
}
|
||||
|
||||
// This test is ignored because lance 3.0 introduced LocalWriter optimization
|
||||
// that bypasses the object store wrapper for local writes. The mirroring feature
|
||||
// still works for remote/cloud storage, but can't be tested with local storage.
|
||||
|
||||
Reference in New Issue
Block a user