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(tidb-engine-ext): Mount the necessary cargo home cache directory #3212

Merged

Conversation

purelind
Copy link
Collaborator

@purelind purelind commented Nov 8, 2024

Mount the necessary cargo home cache directory to reduce cache size.

Copy link

ti-chi-bot bot commented Nov 8, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request title and description, the key change in this pull request is to mount the necessary cargo home cache directory to reduce cache size. Specifically, the diff shows that the latest-presubmits.yaml and release-6.1-latest-presubmits.yaml files have been modified to mount the cache directory with subpaths for git/db, registry/index, and registry/cache.

One potential problem with this pull request is that it removes the commands that delete the existing cache directories and create symbolic links to the new cache directories. It is unclear whether these commands are still necessary with the new changes. It might be helpful to have some documentation explaining why these commands were removed and whether they are still necessary.

Another potential problem is that the pull request does not have any tests or documentation to ensure that the new cache directory is mounted and used correctly. It would be helpful to have some tests that verify that the cache directory is mounted and used correctly.

To fix these issues, the pull request author could add some documentation explaining the changes and why certain commands were removed. Additionally, the author could add tests to ensure that the cache directory is mounted and used correctly.

@ti-chi-bot ti-chi-bot bot added the size/M label Nov 8, 2024
Copy link

ti-chi-bot bot commented Nov 8, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request, the key changes in this PR are that a new directory is being mounted to reduce cache size. Specifically, the necessary cargo home cache directory is being mounted with three subdirectories: git/db, registry/index, and registry/cache.

One potential issue with this PR is that it removes some commands that were previously executed in the presubmits job. Specifically, the two rm commands and two ln -s commands are being removed. It's unclear if these commands were necessary or if they were redundant with the new mount points.

To fix this, it would be helpful to add some comments explaining why those commands were removed and how the new mount points will replace their functionality. Additionally, it would be beneficial to test this change on a staging environment before merging it to the main branch.

Overall, the changes seem reasonable and should help reduce cache size, but it's important to make sure that this change doesn't introduce any unexpected behavior.

@ti-chi-bot ti-chi-bot bot added size/S and removed size/M labels Nov 8, 2024
Copy link

ti-chi-bot bot commented Nov 8, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the title and description of the pull request, it seems that the changes are related to mounting the necessary cargo home cache directory to reduce cache size. The diff shows that in both latest-presubmits.yaml and release-6.1-latest-presubmits.yaml, the code has been modified to add volume mounts for the cargo cache directories and subdirectories.

I don't see any potential problems with this pull request. However, I do have some suggestions for fixing:

  1. The description of the pull request could be more informative. It's better to provide more details about the changes made in the code.

  2. The commit message could be more descriptive, explaining why the change was made and what problem it solves.

  3. It's always a good practice to test the changes locally before pushing to the remote repository. The reviewer could suggest the same to the author.

Overall, the changes seem good and can be merged into the main branch after addressing the above suggestions.

Copy link

ti-chi-bot bot commented Nov 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the lgtm label Nov 8, 2024
Copy link

ti-chi-bot bot commented Nov 8, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-11-08 09:27:07.22883356 +0000 UTC m=+2789.419702556: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot added the approved label Nov 8, 2024
@ti-chi-bot ti-chi-bot bot merged commit f5d8046 into PingCAP-QE:main Nov 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants