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: add retry and check binary #3117

Conversation

purelind
Copy link
Collaborator

add retry and check binary.

@purelind
Copy link
Collaborator Author

/hold

Copy link

ti-chi-bot bot commented Sep 11, 2024

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

Key changes in the pull request:

  1. The pull request includes changes in multiple Groovy files, which are part of a pipeline script.
  2. The make command is now always run to build the tidb-server instead of only when bin/tidb-server does not exist.
  3. The download binary process is now included in a retry(2) block, which means the process will be retried 2 times if it fails.
  4. After downloading the binary, three checks are added to verify the binary files tidb-server, pd-server, and tikv-server. These checks include ensuring the files exist, making them executable, and verifying their versions.

Potential problems:

  1. The make command is now always run even when bin/tidb-server already exists. This might increase the build time unnecessarily if the binary is already up to date.
  2. The mv third_bin/* bin/ command may fail if third_bin does not exist or is empty. It would be better to check if third_bin exists and is not empty before trying to move its contents.
  3. The checks for the binary files assume that the files are always present. If any of the binary files are missing, the corresponding check will fail.

Suggestions for fixes:

  1. Add a condition to check if bin/tidb-server is up-to-date before running the make command.
  2. Check if third_bin exists and is not empty before trying to move its contents.
  3. Add error handling for the case when any of the binary files are missing.

@ti-chi-bot ti-chi-bot bot added the size/L label Sep 11, 2024
Copy link

ti-chi-bot bot commented Sep 11, 2024

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

Summary of Changes:

  1. The ls bin/tidb-server || make command has been replaced with make. This means the tidb-server will always be built, regardless of whether it already exists or not.

  2. The download binary step and a new check binary step have been enclosed within a retry(2) block, meaning these steps will be attempted up to three times if they fail.

  3. The check binary step performs a check on the tidb-server, pd-server, and tikv-server binaries by attempting to list them and print their versions.

  4. The chmod +x commands have been removed from the print version steps. It is assumed that the required permissions are already set on the binaries.

Potential Problems:

  1. If the make command fails or if the binaries are not correctly built for some reason, this will cause the check binary step to fail. This is because the check binary step attempts to list and print the version of the binaries.

  2. If the download binary step fails (for example, due to a network issue), it will now be retried up to two more times. However, if it continues to fail, this will cause the pipeline to fail.

  3. If the binaries do not have the correct permissions set, the print version steps will fail. This could occur if, for example, the binaries were not correctly built or were modified by a previous step.

Suggestions for Fixes:

  1. For the first issue, ensure that the make command is correctly building the binaries. If the make command is failing, investigate the cause of the failure.

  2. For the second issue, ensure that the download binary step is correctly implemented and that the network connection is stable. If the step continues to fail, you may need to investigate the cause of the failure.

  3. For the third issue, consider adding a step to explicitly set the permissions on the binaries before attempting to execute them. This could be done using a chmod +x command.

@wuhuizuo
Copy link
Collaborator

/lgtm
/approve

@ti-chi-bot ti-chi-bot bot added the lgtm label Sep 11, 2024
Copy link

ti-chi-bot bot commented Sep 11, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-09-11 04:09:33.233891146 +0000 UTC m=+415842.974315082: ☑️ agreed by wuhuizuo.

Copy link

ti-chi-bot bot commented Sep 11, 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 Sep 11, 2024
@purelind
Copy link
Collaborator Author

/unhold

@ti-chi-bot ti-chi-bot bot merged commit e41d71a into PingCAP-QE:main Sep 11, 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