Consider refactoring LruCacheLayer with list_with_metakey and concurrent_stat_in_list

[WenyXu]

Consider refactoring it with list_with_metakey and concurrent_stat_in_list

Originally posted by @WenyXu in #3058 (comment)

greptimedb/src/object-store/src/layers/lru_cache/read_cache.rs

Lines 135 to 157 in 2f98fa0

	pub(crate) async fn recover_cache(&self) -> Result<(u64, u64)> {
	let (_, mut pager) = self.file_cache.list("/", OpList::default()).await?;
	while let Some(entry) = pager.next().await? {
	let read_key = entry.path();
	// We can't retrieve the metadata from `[opendal::raw::oio::Entry]` directly,
	// because it's private field.
	let size = {
	let stat = self.file_cache.stat(read_key, OpStat::default()).await?;
	stat.into_metadata().content_length()
	};
	OBJECT_STORE_LRU_CACHE_ENTRIES.inc();
	OBJECT_STORE_LRU_CACHE_BYTES.add(size as i64);
	self.mem_cache
	.insert(read_key.to_string(), ReadResult::Success(size as u32))
	.await;
	}
	Ok(self.stat().await)
	}

[suyanhanx]

I want to try this. Please assign it to me. 😊

[tisonkun]

@suyanhanx Welcome! You're assigned and feel free to leave a comment here to share updates; or ping me as a reviewer if the patch is submitted.

[suyanhanx]

I checked the current implementation and it currently calls mainly on the Accessor.
If we want to refactor it with the new methods list_with_metakey and concurrent_stat_in_list, I think we may need to change it to Operator instead.
This will remove the trait currently relied upon here and its upstream since Operator is not a trait.

greptimedb/src/object-store/src/layers/lru_cache/read_cache.rs

Lines 72 to 110 in 2f98fa0

	impl<C: Accessor + Clone> ReadCache<C> {
	/// Create a [`ReadCache`] with capacity in bytes.
	pub(crate) fn new(file_cache: Arc<C>, capacity: usize) -> Self {
	let file_cache_cloned = file_cache.clone();
	let eviction_listener =
	move |read_key: Arc<String>, read_result: ReadResult, cause| -> ListenerFuture {
	// Delete the file from local file cache when it's purged from mem_cache.
	OBJECT_STORE_LRU_CACHE_ENTRIES.dec();
	let file_cache_cloned = file_cache_cloned.clone();
	async move {
	if let ReadResult::Success(size) = read_result {
	OBJECT_STORE_LRU_CACHE_BYTES.sub(size as i64);
	let result = file_cache_cloned.delete(&read_key, OpDelete::new()).await;
	debug!(
	"Deleted local cache file `{}`, result: {:?}, cause: {:?}.",
	read_key, result, cause
	);
	}
	}
	.boxed()
	};
	Self {
	file_cache,
	mem_cache: Cache::builder()
	.max_capacity(capacity as u64)
	.weigher(|_key, value: &ReadResult| -> u32 {
	// TODO(dennis): add key's length to weight?
	value.size_bytes()
	})
	.async_eviction_listener(eviction_listener)
	.support_invalidation_closures()
	.build(),
	}
	}
	/// Returns the cache's entry count and total approximate entry size in bytes.

[ozewr]

Hi,Can I give it a try?

[WenyXu]

Hi,Can I give it a try?

Have fun

[ozewr]

Can I replace Access trait to Accessdyntrait?

/opendal/src/raw/accessor.rs:398
/// AccessDyn is the dyn version of [Access] make it possible to use as
/// Box<dyn AccessDyn>.

The Operator struct can build from dyn AccessDyn.
This way, I can construct the Operator in the recover_cache function, and then use list_with_metakey and concurrent_stat_in_list.

[WenyXu]

Can I replace Access trait to Accessdyntrait?

/opendal/src/raw/accessor.rs:398
/// AccessDyn is the dyn version of [Access] make it possible to use as
/// Box<dyn AccessDyn>.

The Operator struct can build from dyn AccessDyn. This way, I can construct the Operator in the recover_cache function, and then use list_with_metakey and concurrent_stat_in_list.

Yes, I think we should.