Skip to content

Commit

Permalink
Add check for slow store to be noop and conditionally replace with fa…
Browse files Browse the repository at this point in the history
…st (#670)
  • Loading branch information
blakehatch committed Feb 21, 2024
1 parent f4d9db3 commit e402a10
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 0 deletions.
11 changes: 11 additions & 0 deletions nativelink-store/src/fast_slow_store.rs
Expand Up @@ -19,6 +19,7 @@ use std::sync::Arc;

use async_trait::async_trait;
use futures::{join, FutureExt};
use nativelink_config::stores::StoreConfig;
use nativelink_error::{make_err, Code, Error, ResultExt};
use nativelink_util::buf_channel::{make_buf_channel_pair, DropCloserReadHalf, DropCloserWriteHalf};
use nativelink_util::common::DigestInfo;
Expand All @@ -44,13 +45,23 @@ impl FastSlowStore {
fast_store: Arc<dyn Store>,
slow_store: Arc<dyn Store>,
) -> Self {
let slow_store = if matches!(_config.slow, StoreConfig::noop) {
fast_store.clone()
} else {
slow_store
};

Self { fast_store, slow_store }
}

pub fn fast_store(&self) -> &Arc<dyn Store> {
&self.fast_store
}

pub fn slow_store(&self) -> &Arc<dyn Store> {
&self.slow_store
}

/// Ensure our fast store is populated. This should be kept as a low
/// cost function. Since the data itself is shared and not copied it should be fairly
/// low cost to just discard the data, but does cost a few mutex locks while
Expand Down
30 changes: 30 additions & 0 deletions nativelink-store/tests/fast_slow_store_test.rs
Expand Up @@ -18,6 +18,7 @@ use std::sync::Arc;
use nativelink_error::Error;
use nativelink_store::fast_slow_store::FastSlowStore;
use nativelink_store::memory_store::MemoryStore;
use nativelink_store::noop_store::NoopStore;
use nativelink_util::common::DigestInfo;
use nativelink_util::store_trait::Store;
use rand::rngs::SmallRng;
Expand Down Expand Up @@ -381,4 +382,33 @@ mod fast_slow_store_tests {
);
Ok(())
}

// Regression test for https://github.com/TraceMachina/nativelink/issues/665
#[tokio::test]
async fn slow_store_replaced_by_fast_store_when_noop() -> Result<(), Error> {
let fast_store = Arc::new(MemoryStore::new(&nativelink_config::stores::MemoryStore::default()));
let slow_store = Arc::new(NoopStore::new());
let fast_slow_store_config = nativelink_config::stores::FastSlowStore {
fast: nativelink_config::stores::StoreConfig::memory(nativelink_config::stores::MemoryStore::default()),
slow: nativelink_config::stores::StoreConfig::noop,
};
let fast_slow_store = Arc::new(FastSlowStore::new(
&fast_slow_store_config,
fast_store.clone(),
slow_store.clone(),
));

assert_eq!(
Arc::as_ptr(fast_slow_store.fast_store()),
Arc::as_ptr(&fast_store),
"Fast store should be the same as the fast_slow_store's fast store"
);
assert_eq!(
Arc::as_ptr(fast_slow_store.slow_store()),
Arc::as_ptr(&fast_store),
"Slow store should be replaced by fast store when configured as noop"
);

Ok(())
}
}

0 comments on commit e402a10

Please sign in to comment.