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

ci: centralized job for rocksdb libaries cache #21657

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

auricom
Copy link
Contributor

@auricom auricom commented Sep 11, 2024

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 :

  • once every 15 minutes
  • when rocksdb version may have been changed (git diff contains build.mk)

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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • [ x added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced a script to automate the installation of RocksDB dependencies, simplifying the setup process for developers.
    • Added a caching workflow for RocksDB libraries to optimize build times.
  • Bug Fixes

    • Improved the installation process by removing outdated dependency commands from existing scripts.
  • Refactor

    • Streamlined the GitHub Actions workflows by consolidating dependency management and simplifying cache handling.
  • Chores

    • Updated workflows to enhance maintainability and efficiency in the CI/CD pipeline.

@auricom auricom requested a review from a team as a code owner September 11, 2024 13:09
Copy link
Contributor

coderabbitai bot commented Sep 11, 2024

Walkthrough

Walkthrough

This pull request introduces several changes focused on the installation and management of RocksDB dependencies within GitHub Actions workflows. A new script, install-rocksdb-deps.sh, automates the installation of essential libraries, while existing workflows are updated to utilize this script instead of inline installation commands. Additionally, a new caching workflow for RocksDB libraries is introduced, optimizing build times by managing cached resources effectively. Overall, the changes aim to streamline the installation process and improve the maintainability of the CI/CD pipeline.

Changes

Files Change Summary
.github/scripts/install-rocksdb-deps.sh Added a new script to automate the installation of RocksDB dependencies.
.github/scripts/install-rocksdb.sh Removed inline dependency installation commands, shifting to reliance on the new script.
.github/workflows/build.yml Added a job step for installing RocksDB dependencies; removed previous caching steps.
.github/workflows/cache-rocksdb.yml Introduced a new caching workflow for RocksDB libraries to optimize build times.
.github/workflows/lint.yml Added a step to install RocksDB dependencies and simplified conditional logic for installation.
.github/workflows/test.yml Replaced direct installation commands with a call to the new dependency installation script; removed caching steps.

Possibly related PRs

  • ci: remove nix from gh workflows #21572: Modifications to the install-rocksdb.sh script relate to the new install-rocksdb-deps.sh script, focusing on automating the installation of RocksDB dependencies.
  • ci: rocksdb version in makefile #21654: Introduces a new script, get-rocksdb-version.sh, which dynamically retrieves the RocksDB version, impacting how installation scripts function in relation to version management.

Suggested labels

backport/v0.52.x


Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c57156c and 73a1769.

Files selected for processing (1)
  • .github/workflows/cache-rocksdb.yml (1 hunks)
Additional context used
actionlint
.github/workflows/cache-rocksdb.yml

28-28: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting

(shellcheck)

Additional comments not posted (1)
.github/workflows/cache-rocksdb.yml (1)

40-62: LGTM!

The save-cache-rocksdb job is well-structured and follows best practices:

  • It correctly uses the if condition to run only if the cache is not found.
  • It uses scripts for installing dependencies and RocksDB, which keeps the workflow file clean and maintainable.
  • It saves the RocksDB libraries cache using the actions/cache/save action with the correct paths and key.

The job is approved.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between aa8bf41 and c57156c.

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:

  1. Add the dependency installation commands back to the script.
  2. Ensure that the required libraries are installed in the environment where the script is executed.
  3. Update the documentation to specify the prerequisites for running the script.
.github/workflows/cache-rocksdb.yml (2)

15-37: LGTM!

The check-cache-rocksdb job 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-rocksdb job 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 the env.GIT_DIFF check.

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_DIFF check 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_DIFF check:

Verification successful

Removal of env.GIT_DIFF Check Verified

The removal of the env.GIT_DIFF check 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_DIFF variable 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.yml

Length 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.yml

Length 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.sh script 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-store and test-store-v2 jobs.

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.

.github/workflows/cache-rocksdb.yml Show resolved Hide resolved
@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Sep 11, 2024
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

utACK

@julienrbrt julienrbrt added this pull request to the merge queue Sep 11, 2024
Merged via the queue into main with commit 4fe934e Sep 11, 2024
73 checks passed
@julienrbrt julienrbrt deleted the claude/rocksdb_cache_optim branch September 11, 2024 20:18
mergify bot pushed a commit that referenced this pull request Sep 11, 2024
(cherry picked from commit 4fe934e)

# Conflicts:
#	.github/workflows/test.yml
julienrbrt added a commit that referenced this pull request Sep 11, 2024
)

Co-authored-by: auricom <27022259+auricom@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
alpe added a commit that referenced this pull request Sep 12, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release Type: CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants