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

fix(apps/prod/tekton/configs): speedup building for tikv #928

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented Jan 8, 2024

  • increase timeout from 2h to 2h30m
  • bind workspace to cache cargo home path

…to trigger template

To increase stability for tikv and tiflash building

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
…timeout

from 2h to 2h30m

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link
Contributor

ti-chi-bot bot commented Jan 8, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
The pull request suggests two changes. First, it increases the timeout from 2 hours to 2.5 hours, and second, it binds the workspace to cache cargo home path.

Potential problems:
The changes seem reasonable and do not raise any significant concerns. However, it would be better to consider other factors, such as the impact of increasing the timeout on the overall workflow, before merging the pull request.

Fixing suggestions:
As the changes suggested in the pull request seem reasonable, it can be merged after reviewing the workflow's overall impact.

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind January 8, 2024 03:51
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster size/S labels Jan 8, 2024
…mplates

They have be replaced by public trigger templates.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot added size/XXL and removed size/S labels Jan 8, 2024
Copy link
Contributor

ti-chi-bot bot commented Jan 8, 2024

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

Review

The changes in the pull request mainly fall into two categories:

  1. Increasing build timeouts and adding workspace for caching:

    • The timeout for building TiKV has been increased from 2 hours to 2 hours and 30 minutes.
    • A new workspace binding has been added to cache the cargo home path. This could potentially speed up the build process by reusing previously compiled dependencies.
  2. Removing several files and references:

    • Several YAML files related to build configurations of different components (like TiDB, TiFlash, etc.) have been removed.
    • Corresponding references to these files in the kustomization.yaml have also been removed.

Potential Problems

  1. Removing build configuration files: You have removed several component-specific build configuration files. If these components are no longer required to be built, this might be fine. However, if they are still required, this will break the build process for these components.

  2. Workspace for caching: The new workspace binding seems to be added for caching purpose, but it's not clear if the build scripts or the build environment will actually use this workspace for caching correctly.

Suggestions

  1. If the removed build configuration files are still required, add them back, and make sure they're properly referenced in the kustomization.yaml.

  2. For the new workspace binding for caching, ensure that the build scripts or the build environment will actually use this workspace for caching correctly. Test the build process with this new configuration to see if it actually speeds up the build.

  3. The PR could be improved by providing more context and explanation for the changes, especially for the removal of build configuration files.

Markdown

# Review

The changes in the pull request mainly fall into two categories:

1. Increasing build timeouts and adding workspace for caching:
   - The timeout for building TiKV has been increased from 2 hours to 2 hours and 30 minutes.
   - A new workspace binding has been added to cache the cargo home path. This could potentially speed up the build process by reusing previously compiled dependencies.

2. Removing several files and references:
   - Several YAML files related to build configurations of different components (like TiDB, TiFlash, etc.) have been removed.
   - Corresponding references to these files in the `kustomization.yaml` have also been removed.

# Potential Problems

1. Removing build configuration files: You have removed several component-specific build configuration files. If these components are no longer required to be built, this might be fine. However, if they are still required, this will break the build process for these components.

2. Workspace for caching: The new workspace binding seems to be added for caching purpose, but it's not clear if the build scripts or the build environment will actually use this workspace for caching correctly.

# Suggestions

1. If the removed build configuration files are still required, add them back, and make sure they're properly referenced in the `kustomization.yaml`.

2. For the new workspace binding for caching, ensure that the build scripts or the build environment will actually use this workspace for caching correctly. Test the build process with this new configuration to see if it actually speeds up the build.

3. The PR could be improved by providing more context and explanation for the changes, especially for the removal of build configuration files.

@wuhuizuo
Copy link
Collaborator Author

wuhuizuo commented Jan 8, 2024

/approve

Copy link
Contributor

ti-chi-bot bot commented Jan 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 approved label Jan 8, 2024
@ti-chi-bot ti-chi-bot bot merged commit abf2566 into main Jan 8, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/tikv-cargo-home-pvc branch January 8, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apps env/prod will deploy on the main product cluster size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant