Closed henrikskog closed 4 months ago
Marked as draft as this is not something that is ready to merge.
Thank you for addressing this!
At this stage, I believe implementing the bare minimum is sufficient.
new
new
load_all_buckets
load_all_buckets
load_all_buckets
load_all_buckets
.)A library like moka doesn't seem necessary because there are no repeated references or updates while the application is running (at least for now).
I removed the moka dep and created the cache manually instead
I don't think multiple updates are much of a problem, but it seems better to read the entire cache first and save the entire cache at the end.
I added the location check to the load_bucket
method as well. Now the cache is written at the end of both load_bucket
and load_buckets
. IMO it would be clean to do it at the end of get_bucket_region
at the expense of writing multiple times to file. This way the cache logic is only a concern of that spesific function.
(However, it seems better to be consistent about whether you do it inside or outside load_all_buckets.)
Hmm I'm now reading in the new
method and writing inside the load-bucket methods. I think it's nice to keep it as an internal concern in Client.rs
instead of doing it in app.rs
?
Thanks for the review. Knowing when to panick and when to propagate errors is a bit tricky I think.
Looks good, thanks!
Fixes #14
Run with
RUST_BACKTRACE=1 cargo test test_load_all_buckets -- --nocapture
. This requires that you have an account authenticated with some s3 buckets though. I'll remove the tests if this gets approved. It's just the easiest way I have found to do manual testing.put
is ran. Super inefficient, but does not really matter maybe? The cache is loaded from file at startup.Very very rough and needs a lot of cleanup with logging and error handling probably. I thought it might be smart to hear your thoughts about the approach here before fixing up all the small things with the code. What do you think?