Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mercury): introduce Storable #769

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

el-ev
Copy link
Contributor

@el-ev el-ev commented Dec 18, 2024

  1. Replace the original FileLoadStore trait with some "real abstractions."
  2. Considered sufficient scalability, it should be seamlessly applicable in other scenarios requiring persistent storage.
  3. Ignore the branch name; it has no real performance impact, whether good or bad.
  4. There are also a few other harmless refactorings.
  5. I suppose other utils mods scattered around different directories and crate::hash could be moved to crate::utils as well.

Copy link

vercel bot commented Dec 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mega ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 30, 2024 7:19am

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (6)
  • mercury/src/utils.rs: Evaluated as low risk
  • mercury/src/internal/pack/mod.rs: Evaluated as low risk
  • mercury/src/internal/pack/decode.rs: Evaluated as low risk
  • libra/src/utils/client_storage.rs: Evaluated as low risk
  • mercury/src/internal/pack/cache.rs: Evaluated as low risk
  • mercury/src/internal/index.rs: Evaluated as low risk
Comments suppressed due to low confidence (2)

mercury/src/lib.rs:13

  • [nitpick] The constant name MERCURY_DEFAULT_TMP_DIR should be renamed to MERCURY_DEFAULT_TMP_DIR to follow Rust naming conventions.
pub(crate) const MERCURY_DEFAULT_TMP_DIR: &str = "/tmp/.cache_temp";

mercury/src/internal/pack/cache_object.rs:291

  • [nitpick] The error message could be more descriptive. It should include more details, such as the type of the object being stored and the path.
eprintln!("[{}] Error while storing {:?}: {:?}", std::any::type_name::<Self>(), path, e);

}

fn store(obj: &T, location: &Path) -> Result<(), io::Error> {
if location.exists() {
Copy link
Preview

Copilot AI Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store method in DefaultFileStorageStrategy does not overwrite existing files. This might be unintended behavior if the file needs to be updated.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@Hou-Xiaoxuan
Copy link
Contributor

Hou-Xiaoxuan commented Dec 19, 2024

Currently, the storage for all other projects is located at https://github.com/web3infra-foundation/mega/tree/main/jupiter. Most of it is related to the database, and the LFS section is now local_storage. TheFileLoadStore here is only used for decoding and caching.

@el-ev
Copy link
Contributor Author

el-ev commented Dec 19, 2024

Currently, the storage for all other projects is located at main/jupiter. Most of it is related to the database, and the LFS section is now local_storage. TheFileLoadStore here is only used for decoding and caching.

I see , I wrote it mainly for fun.

There are other code only used for decoding and caching but written for versatility, such as the generic ArcWrapper<T>

@Hou-Xiaoxuan
Copy link
Contributor

This isn't really an issue. I’ve definitely learned a lot from your code, and this PR looks great. I just wanted to point out that the refactor seems to be slowly leaning towards over-engineering. After all, programming is about finding a balance between "over-engineering" and "straightforwardness."

I see , I wrote it mainly for fun.

There are other code only used for decoding and caching but written for versatility, such as the generic ArcWrapper<T>

@MrBeanCpp
Copy link
Contributor

MrBeanCpp commented Dec 19, 2024

There are other code only used for decoding and caching but written for versatility, such as the generic ArcWrapper<T>

No really, we use ArcWrapper<T> just because LruCache doesn't support Arc<T>, so ArcWrapper<T> is a hack, not for generic purpose.
ref: see lru-mem

/// Note that reference-counting smart pointers deliberately do not implement
/// this trait, as it is not clear whether a pointer will drop the referenced
/// content when it is ejected from the cache.

We don't like over-design, as it makes future maintenance difficult.

@Hou-Xiaoxuan
Copy link
Contributor

Hou-Xiaoxuan commented Dec 19, 2024

No really, we use ArcWrapper just because LruCache doesn't support Arc, so ArcWrapper is a hack, not for generic purpose.

That's the reason why we should have ArcWrapper. But I think he means there’s ArcWrapper rather than something like ArcCacheObject.

I wrote that because, when I first implemented it, I wasn’t sure if the CacheObject would work, so I ended up doing some "engineering" just to be sure.

@MrBeanCpp
Copy link
Contributor

Anyway, I don't want to have to navigate through multiple layers to find the code I need during debugging, like in Java Springboot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants