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 RocksDB CI #3125

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Fix RocksDB CI #3125

merged 3 commits into from
Sep 18, 2024

Conversation

v01dstar
Copy link
Contributor

No description provided.

@ti-chi-bot ti-chi-bot bot requested review from purelind and wuhuizuo September 14, 2024 08:08
Copy link

ti-chi-bot bot commented Sep 14, 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 description and changes, it seems like the author has fixed an issue with RocksDB CI. The key change is that the use_tmp variable is removed from the test function definition and calls, and the TEST_TMPDIR environment variable is set to /home/jenks/tmp_dir instead.

As for potential problems, I don't see any issues with the changes themselves. However, it's possible that the removal of use_tmp could cause problems if there are other parts of the codebase that rely on it.

For fixing suggestions, I recommend testing the changes locally and ensuring that all tests pass. Additionally, it might be a good idea to double-check with other team members to make sure that removing use_tmp won't cause any issues in other parts of the codebase.

@ti-chi-bot ti-chi-bot bot added the size/M label Sep 14, 2024
Copy link

ti-chi-bot bot commented Sep 14, 2024

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

Based on the PR title and the diff, it seems that the changes are related to fixing the Continuous Integration (CI) pipeline for RocksDB testing. The key changes involve removing the use of temporary directories for testing, and instead using a fixed directory for testing.

One potential problem is that the removal of temporary directories may affect some RocksDB tests. It would be useful to investigate whether this is the case and if so, whether there is a way to address it.

As for fixing suggestions, it may be helpful to investigate the issue with temporary directories and determine whether there is a way to use them without affecting the tests. Alternatively, if using a fixed directory is the only solution, it may be helpful to document this change and its potential impact on the tests. Additionally, it may be useful to add more comments to the code to explain the changes made.

Copy link

ti-chi-bot bot commented Sep 14, 2024

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

Based on the diff provided, it seems that the changes in this pull request are related to fixing the RocksDB CI tests. Specifically, the changes involve modifying the test function in the rocksdb_ghpr_test.groovy file to remove the use_tmp parameter and always set the TEST_TMPDIR environment variable to /home/jenkins/tmp_dir.

One potential problem with this change is that it may cause issues with certain RocksDB tests that require a temporary directory. It's unclear from the pull request description whether this issue has been considered and addressed.

To fix this potential issue, it may be worth investigating whether there are certain tests that require a temporary directory and modifying the test function to only set TEST_TMPDIR when running tests that don't have this requirement.

Overall, the changes in this pull request seem straightforward and should improve the reliability of the RocksDB CI tests. However, it's important to ensure that all relevant test cases are still being properly evaluated.

Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Copy link

ti-chi-bot bot commented Sep 15, 2024

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

Based on the diff, it seems that this pull request addresses an issue with the CI for RocksDB. The changes consist of removing a variable called "use_tmp" from the "test" function and setting the "TEST_TMPDIR" environment variable to "/home/jenkins/tmp_dir".

There are no obvious potential problems with these changes. However, it is unclear why the "use_tmp" variable was removed, and whether this could cause any issues. Additionally, it is not stated whether "/home/jenkins/tmp_dir" is a safe and appropriate location to set the "TEST_TMPDIR" variable.

Some suggestions to improve this pull request are:

  • Provide more context and details in the pull request description, including the reason for removing the "use_tmp" variable and the safety of setting "TEST_TMPDIR" to "/home/jenkins/tmp_dir".
  • Consider adding more comments in the code to explain the changes and their rationale.
  • Test the changes thoroughly in a staging environment before merging to ensure that they do not cause any unforeseen issues.

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

ti-chi-bot bot commented Sep 18, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-09-18 06:28:07.345814019 +0000 UTC m=+1028957.086237958: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot added the approved label Sep 18, 2024
Copy link
Collaborator

@purelind purelind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link

ti-chi-bot bot commented Sep 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: purelind, 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 merged commit 8cbcf85 into PingCAP-QE:main Sep 18, 2024
2 checks passed
@v01dstar v01dstar deleted the fix-dir branch September 19, 2024 04:35
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.

3 participants