Skip to content

Use RocksDB native parallel testing#3129

Draft
v01dstar wants to merge 1 commit into
PingCAP-QE:mainfrom
v01dstar:change-rocks-ci-parallel
Draft

Use RocksDB native parallel testing#3129
v01dstar wants to merge 1 commit into
PingCAP-QE:mainfrom
v01dstar:change-rocks-ci-parallel

Conversation

@v01dstar

Copy link
Copy Markdown
Contributor

No description provided.

@ti-chi-bot

ti-chi-bot Bot commented Sep 19, 2024

Copy link
Copy Markdown

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

Based on the title and the diff, the key change in this pull request is the usage of RocksDB native parallel testing. The changes are made in the Jenkins pipeline script for CI/CD of RocksDB.

There are no potential problems identified in this pull request.

As for fixing suggestions, it would be good to have more information in the pull request description, like why this change is being made and what benefits it will bring. Additionally, it would be helpful to add more comments in the code to improve readability and maintainability.

@ti-chi-bot

ti-chi-bot Bot commented Sep 19, 2024

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign purelind for approval. For more information see the Code Review Process.

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

Details 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 requested review from purelind and wuhuizuo September 19, 2024 05:22
@ti-chi-bot ti-chi-bot Bot added the size/L label Sep 19, 2024
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
@v01dstar v01dstar force-pushed the change-rocks-ci-parallel branch from 98a2ec7 to b4f6477 Compare September 19, 2024 05:23
@ti-chi-bot

ti-chi-bot Bot commented Sep 19, 2024

Copy link
Copy Markdown

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 "Use RocksDB native parallel testing", it seems that the main change being made is to switch to using RocksDB native parallel testing. However, the pull request description is empty, which makes it hard to understand the context and reasoning behind the change.

Looking at the diff, it seems that some resource limits have been increased for the rust container, and the build function has been removed. Instead, the test function has been updated to include the J=32 flag to parallelize the tests.

There are no immediate potential problems with this pull request, but it would be helpful to have more information in the pull request description to understand the motivation behind these changes. Additionally, it might be helpful to have more context on the current state of the CI/CD pipeline and any issues it is facing that prompted this change.

One suggestion for improvement would be to add more descriptive names to the parallel stages in the x86 section, as the current names (basic, group1, group2, group3, group4, encrypted_env) are not very informative. Additionally, it might be helpful to add some comments to the code to help explain what is happening at each stage.

@v01dstar

Copy link
Copy Markdown
Contributor Author

make check fails in our CI, it requires python3. So, this change will be paused, until we migrate to new build env.

@v01dstar v01dstar marked this pull request as draft September 19, 2024 18:06
@pingcap-cla-assistant

pingcap-cla-assistant Bot commented Jul 1, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant