wip #4117
wip #4117
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test help |
|
@hongyunyan: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
bot
added
the
size/M
📝 WalkthroughWalkthroughTwo integration test scripts modified: one adds 60-second initialization delays before table movement operations, the other consolidates MySQL heavy test groups to uniformly run identical test pairs instead of varied test cases. No functional logic changes beyond these additions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @hongyunyan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request appears to be a work-in-progress focused on debugging or stabilizing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request appears to be a work-in-progress for debugging purposes. No vulnerabilities or significant security issues were found in the provided code changes. However, it introduces hardcoded sleep commands in ddl_for_split_tables_with_merge_and_split/run.sh which can lead to test flakiness, and modifies run_heavy_it_in_ci.sh to run only a single test repeatedly, drastically reducing CI test coverage. These changes should be reverted before merging.
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G01 | ||
| 'ddl_for_split_tables_with_random_move_table' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G02 | ||
| 'ddl_for_split_tables_with_failover' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G03 | ||
| 'cdc move_table in_flight_ddl_during_scheduling checkpoint_race_ddl_crash' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G04 | ||
| 'complex_transaction' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G05 | ||
| 'ddl_for_split_tables_with_merge_and_split syncpoint in_flight_syncpoint_during_scheduling syncpoint_check_ts random_drop_message' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G06 | ||
| 'ddl_for_split_tables_with_random_merge_and_split' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G07 | ||
| 'consistent_compatibility consistent_partition_table consistent_replicate_gbk consistent_replicate_ddl consistent_replicate_basic' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G08 | ||
| 'default_value http_proxies bank ddl_for_split_tables_random_schedule' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G09 | ||
| 'availability resolve_lock merge_table drop_many_tables ddl_for_split_tables' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G10 | ||
| 'consistent_replicate_nfs consistent_replicate_storage_file consistent_replicate_storage_file_large_value consistent_replicate_storage_s3' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G11 | ||
| 'multi_changefeeds ddl_wait ddl_reentrant force_replicate_table multi_source' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G12 | ||
| 'ddl_with_random_move_table' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G13 | ||
| 'tidb_mysql_test fail_over region_merge fail_over_ddl_mix_random_delay' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G14 | ||
| 'fail_over_ddl_mix' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G15 | ||
| 'fail_over_ddl_mix_with_syncpoint' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' |
There was a problem hiding this comment.
This change replaces all MySQL heavy integration tests with a single test case repeated across all groups. This appears to be for debugging and should be reverted before merging, as it would severely reduce test coverage in the CI pipeline.
'api_v2 generate_column many_pk_or_uk multi_source'
# G01
'ddl_for_split_tables_with_random_move_table'
# G02
'ddl_for_split_tables_with_failover'
# G03
'cdc move_table in_flight_ddl_during_scheduling checkpoint_race_ddl_crash'
# G04
'complex_transaction'
# G05
'ddl_for_split_tables_with_merge_and_split syncpoint in_flight_syncpoint_during_scheduling syncpoint_check_ts random_drop_message'
# G06
'ddl_for_split_tables_with_random_merge_and_split'
# G07
'consistent_compatibility consistent_partition_table consistent_replicate_gbk consistent_replicate_ddl consistent_replicate_basic'
# G08
'default_value http_proxies bank ddl_for_split_tables_random_schedule'
# G09
'availability resolve_lock merge_table drop_many_tables ddl_for_split_tables'
# G10
'consistent_replicate_nfs consistent_replicate_storage_file consistent_replicate_storage_file_large_value consistent_replicate_storage_s3'
# G11
'multi_changefeeds ddl_wait ddl_reentrant force_replicate_table multi_source'
# G12
'ddl_with_random_move_table'
# G13
'tidb_mysql_test fail_over region_merge fail_over_ddl_mix_random_delay'
# G14
'fail_over_ddl_mix'
# G15
'fail_over_ddl_mix_with_syncpoint'| main() { | ||
| prepare changefeed | ||
|
|
||
| sleep 60 |
There was a problem hiding this comment.
Using a fixed sleep can make tests flaky if the duration is too short, or slow them down unnecessarily if it's too long. It's better to poll for a specific condition. For example, you could create a loop that checks for the desired state of the changefeed before proceeding.
| fi | ||
| prepare consistent_changefeed | ||
|
|
||
| sleep 60 |
There was a problem hiding this comment.
Similar to the previous comment, this fixed sleep should be replaced with a more deterministic wait. Polling for the specific condition you're waiting for will make the test more reliable and efficient.
|
/test pull-cdc-mysql-integration-heavy-next-gen |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/integration_tests/run_heavy_it_in_ci.sh`:
- Around line 31-64: The mysql_groups array was overwritten with 16 identical
entries of ddl_for_split_tables_with_merge_and_split (duplicating the test
across all groups); revert this temporary debug change by restoring the original
diverse test-case strings into mysql_groups instead of repeating
ddl_for_split_tables_with_merge_and_split, ensuring each group contains the
intended unique test names and that groups remain balanced by cost as described
in the surrounding file documentation; if you needed to run that single test for
debugging, limit the change to a local or temporary branch or a one-off script
rather than replacing the mysql_groups contents in this file.
🧹 Nitpick comments (2)
tests/integration_tests/ddl_for_split_tables_with_merge_and_split/run.sh (2)
57-58: Add a comment explaining why the 60-second delay is necessary.The 60-second sleep is a significant delay that adds to CI execution time. Without context, it's unclear whether this is:
- Waiting for a specific initialization to complete
- A workaround for a race condition
- A temporary debugging measure
Please add a comment explaining the purpose, or consider using a more targeted approach (e.g., polling for a condition with timeout) instead of a fixed sleep.
105-106: Same as above: document the reason for this 60-second delay.This mirrors the sleep added in
main(). If both require the same wait time for the same reason, consider extracting a helper function or adding a shared constant with documentation explaining the requirement.
| mysql_groups=( | ||
| # G00 | ||
| 'api_v2 generate_column many_pk_or_uk multi_source' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G01 | ||
| 'ddl_for_split_tables_with_random_move_table' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G02 | ||
| 'ddl_for_split_tables_with_failover' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G03 | ||
| 'cdc move_table in_flight_ddl_during_scheduling checkpoint_race_ddl_crash' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G04 | ||
| 'complex_transaction' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G05 | ||
| 'ddl_for_split_tables_with_merge_and_split syncpoint in_flight_syncpoint_during_scheduling syncpoint_check_ts random_drop_message' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G06 | ||
| 'ddl_for_split_tables_with_random_merge_and_split' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G07 | ||
| 'consistent_compatibility consistent_partition_table consistent_replicate_gbk consistent_replicate_ddl consistent_replicate_basic' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G08 | ||
| 'default_value http_proxies bank ddl_for_split_tables_random_schedule' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G09 | ||
| 'availability resolve_lock merge_table drop_many_tables ddl_for_split_tables' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G10 | ||
| 'consistent_replicate_nfs consistent_replicate_storage_file consistent_replicate_storage_file_large_value consistent_replicate_storage_s3' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G11 | ||
| 'multi_changefeeds ddl_wait ddl_reentrant force_replicate_table multi_source' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G12 | ||
| 'ddl_with_random_move_table' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G13 | ||
| 'tidb_mysql_test fail_over region_merge fail_over_ddl_mix_random_delay' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G14 | ||
| 'fail_over_ddl_mix' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| # G15 | ||
| 'fail_over_ddl_mix_with_syncpoint' | ||
| 'ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split' | ||
| ) |
There was a problem hiding this comment.
This appears to be a temporary debugging change that should not be merged.
All 16 MySQL heavy test groups have been replaced with the same test case duplicated (ddl_for_split_tables_with_merge_and_split ddl_for_split_tables_with_merge_and_split). This effectively removes CI coverage for all other MySQL integration tests.
Based on the file's own documentation (lines 11-23), each group should contain diverse test cases to ensure coverage, and groups should be balanced by cost. This change:
- Removes coverage for all other MySQL heavy integration tests
- Runs the same test 32 times (16 groups × 2) instead of diverse tests
- Contradicts the documented purpose of this file
If this is for debugging the ddl_for_split_tables_with_merge_and_split test, please ensure the original test configurations are restored before merging.
🤖 Prompt for AI Agents
In `@tests/integration_tests/run_heavy_it_in_ci.sh` around lines 31 - 64, The
mysql_groups array was overwritten with 16 identical entries of
ddl_for_split_tables_with_merge_and_split (duplicating the test across all
groups); revert this temporary debug change by restoring the original diverse
test-case strings into mysql_groups instead of repeating
ddl_for_split_tables_with_merge_and_split, ensuring each group contains the
intended unique test names and that groups remain balanced by cost as described
in the surrounding file documentation; if you needed to run that single test for
debugging, limit the change to a local or temporary branch or a one-off script
rather than replacing the mysql_groups contents in this file.
1 participant
What problem does this PR solve?
Issue Number: close #xxx
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit