dispatcher: fix the bug during remove dispatcher #1364
Conversation
There was a problem hiding this comment.
Hello @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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.
This PR, marked as '[WIP]', appears to be focused on testing scenarios related to the MySQL sink's batch DML functionality, specifically when batching is not used (batch_update_to_no_batch). The primary functional changes involve modifying the logic that determines whether batch SQL should be generated for DML events in the MySQL sink. Additionally, the PR includes changes to the CI configuration to repeatedly run the batch_update_to_no_batch integration test, likely to thoroughly test this specific scenario.
There are also minor logging adjustments in the dispatcher and maintainer components.
Highlights
- MySQL Sink Batching Logic: The core logic in
pkg/sink/mysql/mysql_writer_dml.gothat decides whether to generate batch SQL (shouldGenBatchSQL) has been modified. The function now accepts the fullConfigobject instead of just thesafemodeboolean, allowing for more configuration-driven decisions about batching. Comments and commented-out code suggest potential future logic based onBatchDMLEnable. - CI Test Configuration: The CI configuration script
tests/integration_tests/run_light_it_in_ci.shhas been updated to replace many existing test entries with repeated runs of thebatch_update_to_no_batchintegration test across multiple groups. This indicates a focused effort to test this specific scenario extensively. - Logging Improvements: Minor logging changes were made in
downstreamadapter/dispatcher/helper.goandmaintainer/barrier.goto provide more specific information in log messages.
Changelog
Click here to see the changelog
- downstreamadapter/dispatcher/helper.go
- Modified log message in
Setfunction to printtaskDispatcherIDinstead of the fulltaskobject (line 49).
- Modified log message in
- maintainer/barrier.go
- Added a new log message "hyy handle block status send dispatcher status" including
dispatcherStatusin theHandleStatusfunction (line 165).
- Added a new log message "hyy handle block status send dispatcher status" including
- pkg/sink/mysql/mysql_writer_dml.go
- Updated the call to
shouldGenBatchSQLinprepareDMLsto pass thew.cfgobject instead of justw.cfg.SafeMode(line 75). - Changed the signature of
shouldGenBatchS 8000 QLto accept*Configinstead of a booleansafemode(line 99). - Updated comments and added commented-out code in
shouldGenBatchSQLrelated toBatchDMLEnableand event length checks (lines 94-106). - Modified the call to
allRowInSameSafeModewithinshouldGenBatchSQLto usecfg.SafeMode(line 116).
- Updated the call to
- tests/integration_tests/run_light_it_in_ci.sh
- Replaced most entries in the
mysql_groupsarray with repeated occurrences of thebatch_update_to_no_batchtest name (lines 35-65).
- Replaced most entries in the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Code changes arrive,
Tests run to make sure they thrive,
Batching logic shifts.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR appears to be a work-in-progress, focusing on testing the batch_update_to_no_batch functionality. The changes primarily involve modifications to logging, conditional logic for batch SQL generation, and significant alterations to the CI test script for MySQL integration tests.
Key areas to address before this PR moves out of WIP status:
- The PR description (Issue Number, What is changed, Tests, Questions, Release note) needs to be thoroughly filled out.
- Temporary/debug code (like specific log prefixes or commented-out logic) should be cleaned up.
- The CI test script modifications must be reverted to ensure full test coverage is restored.
Overall, the direction seems clear for testing a specific feature. Good job isolating the test case in the CI script for focused testing during development!
Summary of Findings
- CI Test Script Modification: The
run_light_it_in_ci.shscript has been modified to run only thebatch_update_to_no_batchtest for all MySQL groups. This must be reverted before final merge. - Temporary Debug Log: A log message with a personal prefix "hyy" was found in
maintainer/barrier.go. This should be removed or standardized. - Commented-Out Logic: In
pkg/sink/mysql/mysql_writer_dml.go, there's commented-out logic related toBatchDMLEnableand single event checks. TheBatchDMLEnablecheck might need to be uncommented, and the single event check appears redundant. - Logging Improvement (Not Commented): In
downstreamadapter/dispatcher/helper.go, changing the logging of thetaskobject totask.dispatcher.idis a good improvement for conciseness. This was not commented on directly due to review severity settings.
Merge Readiness
This pull request is currently a Work-In-Progress and is not ready for merging. Several critical and medium severity issues need to be addressed:
- The CI test script (
tests/integration_tests/run_light_it_in_ci.sh) modifications must be reverted to ensure full test coverage. - Temporary debug logs (e.g., in
maintainer/barrier.go) should be removed. - Commented-out logic in
pkg/sink/mysql/mysql_writer_dml.goneeds clarification and potential uncommenting/cleanup. - The pull request description, including issue numbers and release notes, needs to be completed.
I recommend addressing these points before the PR is considered for merging. As an AI, I am not authorized to approve pull requests; please ensure other human reviewers approve this code before merging.
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G01 | ||
| 'common_1 foreign_key changefeed_pause_resume fail_over_ddl_B' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G02 | ||
| 'new_ci_collation safe_mode savepoint fail_over_ddl_C' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G03 | ||
| 'capture_suicide_while_balance_table kv_client_stream_reconnect fail_over_ddl_D' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G04 | ||
| 'ci_collation_compatibility multi_capture resourcecontrol fail_over_ddl_E' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G05 | ||
| 'vector simple partition_table fail_over_ddl_F' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G06 | ||
| 'http_api http_api_tls ddl_wait fail_over_ddl_G' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G07 | ||
| 'http_api_tls_with_user_auth fail_over_ddl_H' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G08 | ||
| 'capture_session_done_during_task changefeed_dup_error_restart mysql_sink_retry fail_over_ddl_I' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G09 | ||
| 'cdc_server_tips ddl_sequence server_config_compatibility fail_over_ddl_J' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G10 | ||
| 'changefeed_error bdr_mode fail_over_ddl_K' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G11 | ||
| 'multi_tables_ddl ddl_attributes multi_cdc_cluster fail_over_ddl_L' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G12 | ||
| 'row_format tiflash multi_rocks fail_over_ddl_M' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G13 | ||
| 'cli_tls_with_auth cli_with_auth fail_over_ddl_N' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G14 | ||
| 'batch_add_table batch_update_to_no_batch fail_over_ddl_O' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' | ||
| # G15 | ||
| 'split_region changefeed_resume_with_checkpoint_ts autorandom gc_safepoint' | ||
| 'batch_update_to_no_batch batch_update_to_no_batch batch_update_to_no_batch' |
There was a problem hiding this comment.
This change replaces all MySQL integration test groups with repetitions of batch_update_to_no_batch. While this is understandable for focused testing in a WIP PR, it's critical that this is reverted before merging the final PR to ensure all other MySQL integration tests are executed. Please make a note to restore the original test groups.
maintainer/barrier.go
Outdated
| @@ -161,6 +161,8 @@ func (b *Barrier) HandleStatus(from node.ID, | |||
| zap.String("changefeed", request.ChangefeedID.String())) | |||
| return nil | |||
| } | |||
|
|
|||
| log.Info("hyy handle block status send dispatcher status", zap.Any("dispatcherStatus", dispatcherStatus)) | |||
There was a problem hiding this comment.
This log message with the prefix "hyy" seems like a temporary debug statement. Could you please remove this log line if it's only for debugging purposes, or update the message to be more descriptive and remove the prefix if it's intended to be a permanent log?
pkg/sink/mysql/mysql_writer_dml.go
Outdated
| // if !cfg.BatchDMLEnable { | ||
| // return false | ||
| // } | ||
|
|
||
| // if len(events) == 1 && events[0].Len() == 1 { | ||
| // return false | ||
| // } |
There was a problem hiding this comment.
There's some commented-out logic here.
- The condition
!cfg.BatchDMLEnable(lines 100-102) aligns with the updated function comment: "1. BatchDMLEnable = true, and rows > 1". Should this be uncommented to enforce this check? - The condition
len(events) == 1 && events[0].Len() == 1(lines 104-106) appears to be redundant, as this exact check is already active later in the function at lines 108-110 (previously lines 112-114 in the diff).
Could you clarify the intent here? If the BatchDMLEnable check is needed, it should be uncommented. The redundant check for single row events should probably be removed from this commented block.
if !cfg.BatchDMLEnable {
return false
}
// The following check is redundant as it's handled below.
// if len(events) == 1 && events[0].Len() == 1 {
// return false
// }|
@gemini-code-assist[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files. In 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. |
|
/retest |
|
/test help |
|
@hongyunyan: The specified target(s) for The following commands are available to trigger optional jobs: Use In 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. |
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen 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 |
[LGTM Timeline notifier]Timeline:
|
|
Hello @hongyunyan, Thanks for tagging me. I've reviewed the latest changes in this pull request. The patches primarily focus on refining the dispatcher closing logic in I noticed that the temporary debug log I mentioned in my initial review (Comment 1) in However, my previous review also highlighted two other important points:
Additionally, I see that some tests failed after the I see that the PR has been approved and LGTM'd. Please consider addressing the remaining points regarding the CI script, PR description, and test failures before merging to ensure the highest quality. Let me know if you have any further questions! |
|
/retest |
|
/test pull-cdc-mysql-integration-heavy |
|
//run-check-issue-triage-complete |
|
/run-check-issue-triage-complete |
|
@hongyunyan: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: close #1356
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