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

Use RocksDB native parallel testing #3129

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

v01dstar
Copy link
Contributor

No description provided.

Copy link

ti-chi-bot bot commented Sep 19, 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 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.

Copy link

ti-chi-bot bot commented Sep 19, 2024

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

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

Copy link

ti-chi-bot bot commented Sep 19, 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 "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
Contributor Author

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

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