cache: better cache system #4951
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 95737d8 in 1 minute and 40 seconds
More details
- Looked at
60lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_8ZpzV2rxHNZSksy3
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Deploying windmill with
|
| Latest commit: |
682337e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://625619c6.windmill.pages.dev |
| Branch Preview URL: | https://uael-cache-validate.windmill.pages.dev |
uael
force-pushed
the
uael/cache_validate
branch
from
95737d8 to
0146432
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0146432 in 1 minute and 5 seconds
More details
- Looked at
1163lines of code in7files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. backend/windmill-common/src/cache.rs:148
- Draft comment:
When thethreadlocal_cachefeature is enabled, the cache key should include theThreadId. Consider modifying the key creation to:
let key = if cfg!(feature = "threadlocal_cache") { (std::thread::current().id(), key.clone()) } else { key };
- Reason this comment was not posted:
Comment looked like it was already resolved.
2. backend/windmill-common/src/cache.rs:317
- Draft comment:
Consider adding more context to the error messages inunwrap_or_errorto improve debugging. For example, include the database query or the expected data format. - Reason this comment was not posted:
Confidence changes required:50%
Theunwrap_or_errorfunction is used to handle errors when fetching data from the database. However, it does not provide detailed error messages, which can make debugging difficult. Adding more context to the error messages would improve the debugging experience.
Workflow ID: wflow_HAqFtiAKJTWuKjKP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a9976c5 in 41 seconds
More details
- Looked at
230lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. backend/windmill-common/src/cache.rs:7
- Draft comment:
Update the comment to reflect the feature name change fromthreadlocal_cachetoscoped_cache. This change should be applied throughout the file where applicable. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. backend/windmill-common/src/cache.rs:623
- Draft comment:
Update the comment to reflect the feature name change fromthreadlocal_cachetoscoped_cache. This change should be applied throughout the file where applicable. - Reason this comment was not posted:
Marked as duplicate.
3. backend/windmill-common/src/cache.rs:896
- Draft comment:
Update the comment to reflect the feature name change fromthreadlocal_cachetoscoped_cache. This change should be applied throughout the file where applicable. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_DVdpvRcsNNd5pkvW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f61ea26 in 1 minute and 0 seconds
More details
- Looked at
201lines of code in6files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_XrVYxyRPfJ6YJLHZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2977334 in 45 seconds
More details
- Looked at
63lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-common/src/cache.rs:524
- Draft comment:
Consider adding a validation step before returning the data to ensure data integrity as described in the PR. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code already has several validation steps: 1) It uses unwrap_or_error to validate the database result exists 2) The SQL query uses type annotations to validate types 3) The RawScript struct enforces data structure. Adding more validation would be redundant. The comment is vague and doesn't specify what validation is missing.
I could be missing some edge cases or data integrity concerns that aren't obvious from just this code. The PR description might provide more context about specific validation needs.
Even if there are edge cases, the comment is too vague to be actionable. If there are specific validation concerns, they should be clearly stated with concrete examples.
Delete the comment since it is vague, not clearly actionable, and the code already has reasonable validation through type checking and error handling.
Workflow ID: wflow_8ZbYeTsmQ8JK27xe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 494e71d in 39 seconds
More details
- Looked at
78lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/.sqlx/query-0bf123446bebbc357c58a53a9319f4954dbf3225e91cbe999e5b264c1a747664.json:1
- Draft comment:
This file appears to be a generated SQLx query description and should not be included in the repository. Consider adding it to .gitignore. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
SQLx query files are actually meant to be committed to version control - they're not generated artifacts that should be ignored. They contain important type information that SQLx uses for compile-time query checking. The comment is suggesting incorrect practice that would break SQLx's compile-time checking feature.
Maybe there are cases where teams prefer to regenerate these files during build time instead of committing them?
While possible, the standard practice with SQLx is to commit these files as they provide compile-time guarantees. Regenerating at build time would defeat this purpose and is not the intended usage.
The comment should be deleted as it suggests incorrect practice that would break SQLx's compile-time checking feature.
Workflow ID: wflow_q8MDdWz6vVs9RAjA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 682337e in 26 seconds
More details
- Looked at
75lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-common/src/cache.rs:55
- Draft comment:
Usingtempfilefor temporary directories is a good choice. Ensure all usages ofmktempare replaced and tested. - Reason this comment was not posted:
Confidence changes required:20%
Thetempfilecrate is used to replacemktempfor creating temporary directories. This change is consistent with the PR description and should work as expected sincetempfileis a well-known crate for handling temporary files and directories in Rust.
Workflow ID: wflow_wFbdYWzTqwT72sC3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Enhances cache system with validation logic and thread-local caching for testing, adding a
validate()method and modifying import logic.validate()method tofs::Bundletrait incache.rsfor data integrity.validate()inScriptData,Option<T>, and(A, B)to check non-empty code and valid data.import_or_insert_with()infsmodule to usevalidate(), logging a warning if validation fails.get_or_insert_async()inFsBackedCacheto check validation before returning data.threadlocal_cachefeature toCargo.tomlfor testing.backend-test.ymlto includethreadlocal_cachein test features.This description was created by
for 682337e. It will automatically update as commits are pushed.