Skip to content

Conversation

@wlwilliamx
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #12371

What is changed and how it works?

This PR fixes a data race detected in the cdc/redo test suite.

Changes:

  1. cdc/redo/meta_manager_test.go:

    • Removed the code block from TestGCAndCleanup that sets redo.DefaultGCIntervalInMs to 20 and defers its reset. This stops the global variable from being written to inside a parallel test.
  2. cdc/redo/main_test.go:

    • Added the github.com/pingcap/tiflow/pkg/redo import.
    • Moved the logic to set redo.DefaultGCIntervalInMs = 20 into the TestMain function.
    • A defer statement is used in TestMain to restore the original value after all tests in the cdc/redo package have finished running.

By moving this logic to TestMain, the global variable is set once at the beginning of the entire package's test run, before any parallel tests are executed. This provides a stable test environment for all tests in the package while completely eliminating the data race.

Check List

Tests

  • Unit test
  • Integration test

Questions

Will it cause performance regression or break compatibility?

None

Do you need to update user documentation, design documentation or monitoring documentation?

None

Release note

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/cherry-pick-not-approved release-note-none Denotes a PR that doesn't merit a release note. labels Oct 20, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 20, 2025

This cherry pick PR is for a release branch and has not yet been approved by triage owners.
Adding the do-not-merge/cherry-pick-not-approved label.

To merge this cherry pick:

  1. It must be approved by the approvers firstly.
  2. AFTER it has been approved by approvers, please wait for the cherry-pick merging approval from triage owners.

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.

@gemini-code-assist
Copy link

Summary of Changes

Hello @wlwilliamx, 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 resolves a data race issue identified in the cdc/redo test suite. The core of the fix involves centralizing the setup and teardown of a global configuration variable, redo.DefaultGCIntervalInMs, into the TestMain function. This ensures that the variable is initialized consistently and safely before any parallel tests run, thereby preventing race conditions and establishing a more stable and predictable testing environment for the package.

Highlights

  • Data Race Fix: Addresses a data race detected in the cdc/redo test suite by ensuring the global variable redo.DefaultGCIntervalInMs is set once before any parallel tests are executed.
  • Test Setup Refactor: Relocates the modification and restoration of the global variable redo.DefaultGCIntervalInMs from TestGCAndCleanup to the TestMain function within the cdc/redo package.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

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

@ti-chi-bot ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 20, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly fixes a data race in TestGCAndCleanup by moving the modification of a global variable to the TestMain function. This is the standard and correct way to handle package-level test setup, ensuring that the global state is modified only once before any parallel tests are run. The change is effective and well-implemented. I have one minor suggestion to improve code readability.


func TestMain(m *testing.M) {
originValue := redo.DefaultGCIntervalInMs
redo.DefaultGCIntervalInMs = 20

Choose a reason for hiding this comment

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

medium

The value 20 is a magic number. To improve readability and maintainability, it's better to define it as a named constant. This makes the purpose of the value clearer, especially since it's used to configure a test-specific behavior for the whole package.

	const testDefaultGCIntervalInMs = 20
	redo.DefaultGCIntervalInMs = testDefaultGCIntervalInMs

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Oct 20, 2025
@ti-chi-bot ti-chi-bot bot added the lgtm label Oct 20, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lidezhu, wk989898

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 removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 20, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 20, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-10-20 04:39:39.055744818 +0000 UTC m=+673885.132997377: ☑️ agreed by lidezhu.
  • 2025-10-20 06:17:59.763665512 +0000 UTC m=+679785.840918062: ☑️ agreed by wk989898.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved do-not-merge/cherry-pick-not-approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants