Remove FileStore struct from file_store #1048
Conversation
| pub async fn run(&self, settings: &Settings) -> Result { | ||
| let store = FileStore::from_settings(settings).await?; | ||
| let mut file_infos = self.filter.list(&store); | ||
| let client = settings.connect().await; |
There was a problem hiding this comment.
As for me, having connect function in settings module looks awkward.
I think I'd be nice to move it out of Settings module
There was a problem hiding this comment.
I was following the pattern we have established with db_store, I thought it would feel good for both to follow the same pattern.
file_store/src/file_info_poller.rs
Outdated
| } | ||
|
|
||
| impl<Message, State, Parser> ManagedTask for FileInfoPollerServer<Message, State, FileStore, Parser> | ||
| impl<Message, State, Parser> ManagedTask |
There was a problem hiding this comment.
I don't remember why I hardcoded FileStore in here. But maybe we want to make this generic over Store like the impl block below.
That way someone could still use task-manager if they decide to implement FileInfoPollerStore themselves.
file_store/src/file_upload.rs
Outdated
| .map(|msg| (self.client.clone(), self.bucket.clone(), msg)) | ||
| .for_each_concurrent(5, |(client, bucket, path)| async move { |
There was a problem hiding this comment.
Potential for future updates.
This function takes self, we can grab references to instead of cloning.
let client = &self.client;
let bucket = &self.bucket;That would allow us to remove the .map() call.
There was a problem hiding this comment.
refactored eabefc1
file_store/src/lib.rs
Outdated
|
|
||
| pub fn list_files<A, B>( | ||
| client: &Client, | ||
| bucket: &str, |
There was a problem hiding this comment.
similar to FileInfoPollerConfigBuilder::file_store(client, bucket), may be nice to mirror aws' bucket method and accept Into<String> for all these functions..
pub fn bucket(mut self, input: impl ::std::convert::Into<::std::string::String>) -> Self {
self.inner = self.inner.bucket(input.into());
self
}There was a problem hiding this comment.
good call, refactored beffa76
file_store/src/lib.rs
Outdated
| Ok(stream) => stream_source(stream), | ||
| Err(err) => stream::once(async move { Err(err) }).boxed(), | ||
| }) | ||
| // .fuse() |
There was a problem hiding this comment.
Why was this .fuse() removed?
There was a problem hiding this comment.
mistake, was testing something and forgot to uncomment
| crate::new_client( | ||
| self.endpoint.clone(), | ||
| self.access_key_id.clone(), | ||
| self.secret_access_key.clone(), | ||
| ) |
There was a problem hiding this comment.
Our production happy path being to call this function where we expect all the settings to not be provided makes me feel like we're missing an abstraction. I'm not sure what. But I'll be thinking.
There was a problem hiding this comment.
it is a fair point, in our production settings we inherit all settings to connect to s3 from the environment. These properties are only necessary when doing local testing.
| use std::{path::Path, time::Duration}; | ||
|
|
||
| #[derive(Debug, Deserialize, Clone)] | ||
| pub struct Buckets { |
price/src/main.rs
Outdated
| // Install the prometheus metrics exporter | ||
| poc_metrics::start_metrics(&settings.metrics)?; | ||
|
|
||
| let s3_client = settings.file_store.connect().await; |
There was a problem hiding this comment.
Everywhere else this is file_store_client.
bbalser
force-pushed
the
bbalser/file-store-refactor
branch
from
5046c0a to
14851d3
Compare
An issue was identified when there serveral instances of aws_sdk_s3::Client created when the credentials are refreshed.
When trying to refresh credentials it will sometimes error saying no credentials found. The best fix identified was to create a single aws_sdk_s3::Client and clone it rather than creating new ones per bucket.
To address this, the FileStore struct was deleted and all low level methods from it have been moved to free functions on the file_store crate. These low level functions now take a file_store::Client and bucket as arguments.