-
Notifications
You must be signed in to change notification settings - Fork 472
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
Enable CI for kvrocks2redis #2175
Conversation
aaf4a1f
to
274a26d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Can you create the issues to trace the failed test?
Redis-server compile failed in m1 mac(macos-14), Maybe we can disable install it in macos-14 runner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
It could be very helpful if you can open an issue for the test failure, with asan log. |
e696816
to
7514593
Compare
I enable the CI for macos and with tsan/asan, we can check the failure first. |
20726e6
to
aa8e555
Compare
Hi @Zakelly, you don't need to use force push to make sure commit only one. |
Here's one example result for enabling CI in all envs. https://github.com/Zakelly/kvrocks/actions/runs/8344973371/job/22838786340 |
No worry. You can firstly work on this PR and make it merged, with ASAN/TSAN/macos.. disabled. And then feel free to investigate the memory issue in kvrocks2redis, if you are interested. |
009d772
to
47ec299
Compare
Created #2195 to track CI problem with tsan/asan and macos. Disabled for them.... |
.github/workflows/kvrocks.yaml
Outdated
sleep 15s | ||
python3 utils/kvrocks2redis/tests/populate-kvrocks.py --password="" --flushdb=true | ||
sleep 15s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should it be 15s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wait here to make sure the daemonized processes are initialized, or the background process finishes its work. 10s also works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change it to 10s to save some CI time : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, changed to 10s. : )
And I found the |
I think it is OK! @Zakelly |
@jihuayu @PragmaTwice I think this PR is in good shape, and all the CI has passed for commit 2246fac: https://github.com/Zakelly/kvrocks/actions/runs/8589761419 . Please let me know if you have any other suggestions or comments, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your persistence and efforts.
Quality Gate passedIssues Measures |
This fixes #1976 and fixes #1338 . Add CI for kvrocks2redis.
Limitation:
So CI disabled for these env for now.