ci: centralized job for rocksdb libaries cache #21657
Conversation
WalkthroughWalkthroughThis pull request introduces several changes focused on the installation and management of RocksDB dependencies within GitHub Actions workflows. A new script, Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional context usedactionlint
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- .github/scripts/install-rocksdb-deps.sh (1 hunks)
- .github/scripts/install-rocksdb.sh (1 hunks)
- .github/workflows/build.yml (1 hunks)
- .github/workflows/cache-rocksdb.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- .github/workflows/test.yml (2 hunks)
Files skipped from review due to trivial changes (1)
- .github/scripts/install-rocksdb-deps.sh
Additional context used
actionlint
.github/workflows/cache-rocksdb.yml
27-27: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
Additional comments not posted (8)
.github/scripts/install-rocksdb.sh (1)
Line range hint
1-18: Verify the impact of removing the dependency installation commands.The removal of the dependency installation commands could lead to issues if the required libraries are not present in the environment where the script is executed.
Please ensure that the script works as expected by running the following verification script:
If the script fails, consider the following solutions:
- Add the dependency installation commands back to the script.
- Ensure that the required libraries are installed in the environment where the script is executed.
- Update the documentation to specify the prerequisites for running the script.
.github/workflows/cache-rocksdb.yml (2)
15-37: LGTM!The
check-cache-rocksdbjob is correctly implemented. It checks if the RocksDB cache exists and passes the result to the next job.Tools
actionlint
27-27: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
39-60: LGTM!The
save-cache-rocksdbjob is correctly implemented. It runs only if the cache doesn't exist, installs RocksDB dependencies and libraries, and saves the cache..github/workflows/build.yml (1)
41-43: LGTM!The new job step to install RocksDB dependencies for the amd64 architecture is a necessary addition to centralize the management of the RocksDB cache. The conditional execution based on the architecture is correct.
.github/workflows/lint.yml (2)
42-43: LGTM!The new step to install RocksDB dependencies using a dedicated script is a good addition. It enhances the setup process by ensuring that necessary dependencies are installed prior to the main RocksDB installation.
45-45: LGTM, but verify the impact of removing theenv.GIT_DIFFcheck.The simplification of the conditional logic for the RocksDB installation is a good change. It streamlines the control flow and potentially reduces unnecessary installations when the cache is available.
However, please verify that removing the
env.GIT_DIFFcheck does not have any unintended consequences. Ensure that the RocksDB installation should always run if the cache is not hit, regardless of the changes in the repository.Run the following script to verify the impact of removing the
env.GIT_DIFFcheck:Verification successful
Removal of
env.GIT_DIFFCheck VerifiedThe removal of the
env.GIT_DIFFcheck from the RocksDB installation condition does not impact other parts of the workflow. The change simplifies the condition to depend solely on the cache hit status, aligning with the intention to streamline the workflow. No other references to RocksDB were found in the workflow file.
- The
env.GIT_DIFFvariable is used in other conditions but not related to RocksDB.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `env.GIT_DIFF` check on the RocksDB installation. # Test: Search for the usage of `env.GIT_DIFF` in the workflow file. # Expect: No other steps rely on `env.GIT_DIFF` for the RocksDB installation. rg --type yaml $'env\.GIT_DIFF' .github/workflows/lint.ymlLength of output: 294
Script:
#!/bin/bash # Description: Check for any references to RocksDB in the workflow file to ensure no unintended consequences from removing the `env.GIT_DIFF` check. # Test: Search for any references to RocksDB in the workflow file. rg --type yaml 'RocksDB' .github/workflows/lint.ymlLength of output: 51
.github/workflows/test.yml (2)
804-804: Approve the use of a separate script for installing RocksDB dependencies.Abstracting the installation logic for RocksDB dependencies into the
install-rocksdb-deps.shscript improves the maintainability and readability of the workflow. The script is used consistently across the relevant jobs, which is a good practice.Also applies to: 854-854
Line range hint
804-858: Clarify the reasoning behind removing the caching steps for RocksDB libraries.The caching steps for RocksDB libraries, including the conditional checks for the success of the RocksDB installation and the associated cache save actions, have been removed from the
test-storeandtest-store-v2jobs.Could you please provide more context on the decision to remove the caching steps? Removing caching may impact the build times and efficiency of the CI/CD pipeline. It would be helpful to understand the reasoning behind this change and any potential trade-offs or mitigations considered.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 4fe934e) # Conflicts: # .github/workflows/test.yml
* main: docs(client/debug): correct `debug raw-bytes` command example (#21671) build: don't reinstall golangci-lint if already installed (#21662) refactor(server/v2): kill viper from server components (#21663) chore: sync changelog with latest releases (#21658) refactor: remove viper as a direct dependency (#21635) ci: centralized job for rocksdb libaries cache (#21657) fix: remove stray fmt.Println (#21661)
Description
RocksDB cache is now managed in a centralized job, to avoid concurrency issues when multiple jobs tries to save a cache using the same key.
This job is executed :
RocksDB libraries installation and cache saving only occurs when cache is not alive.
Tested on my fork :
execution #1 : does not find cache, builds rocks, save cache. duration : 10mn
execution #2 : does find the cache, exit. duration : 20s
There is a fallback on jobs requiring rocksdb libraries - if centralized job did not have the time to build the cache, installation will be done locally.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores