master(dm): avoid slow OpenAPI delete on unavailable downstream

[GMHDBJD]

What problem does this PR solve?

Issue Number: close #12562

What is changed and how it works?

• use a 10s downstream connection timeout for OpenAPI delete
• let task deletion continue when downstream metadata cleanup fails
• log skipped downstream metadata cleanup instead of failing the delete request
• simplify the related OpenAPI integration case to use failpoint

Check List

Tests

• Unit test: make dm_unit_test_pkg PKG=github.com/pingcap/tiflow/dm/master
• Integration test: tests/run.sh openapi (currently blocked by an unrelated existing failure in test_multi_tasks: ERROR 1236 (HY000): Client requested master to start replication from position > file size)

Questions

Will it cause performance regression or break compatibility?

No. It only changes the OpenAPI delete path when downstream is unavailable.

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

No.

Release note

Fix slow DM OpenAPI task deletion when the downstream is unavailable.

[ti-chi-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign d3hunter for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• dm/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 addresses a critical issue where OpenAPI task deletion could become excessively slow or hang indefinitely if the downstream database was unavailable or unresponsive. The changes introduce a specific timeout for downstream connections during deletion and modify the error handling to allow task deletion to proceed even if downstream metadata cleanup encounters issues. This significantly improves the reliability and responsiveness of the task deletion API, ensuring that tasks can be removed promptly without being blocked by external system states.

Highlights

• Improved Task Deletion Resilience: The OpenAPI task deletion process has been made more robust by allowing it to continue even if downstream metadata cleanup fails or the downstream database is unavailable.
• Configurable Downstream Connection Timeout: A 10-second timeout has been introduced for downstream database connections during OpenAPI task deletion, preventing indefinite waits when the downstream is unresponsive.
• Enhanced Error Handling: Failed attempts to clean up downstream metadata during task deletion will now be logged as warnings instead of causing the entire deletion request to fail, ensuring a smoother user experience.
• Refactored Metadata Removal Logic: The metadata removal functions have been refactored to separate internal metadata cleanup from downstream metadata cleanup, and to allow for timeout configurations.
• Updated Test Cases: Existing integration tests for task deletion with a stopped downstream have been updated, and a new test case has been added to specifically verify the behavior when downstream metadata cleanup fails using failpoints.

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

Changelog

• dm/master/openapi_controller.go
  • Added a 10-second timeout constant for OpenAPI task deletion downstream connections.
  • Modified the deleteTask function to use the new timeout for downstream database operations.
  • Updated deleteTask to log warnings for failed downstream metadata cleanup instead of returning an error, allowing the deletion process to continue.
  • Refactored the defer release() mechanism in deleteTask to ensure proper resource release.
• dm/master/openapi_controller_test.go
  • Updated TestTaskController to incorporate failpoints for simulating downstream metadata cleanup errors during task deletion, aligning with the new error handling.
• dm/master/server.go
  • Introduced AdjustTargetDBSessionCfgWithTimeout to allow specifying a timeout for database session configuration.
  • Refactored removeMetaData into removeMetaDataWithTimeout, removeInternalMetaData, and removeDownstreamMetaData to provide more granular control and timeout capabilities.
  • Updated calls to database connection functions to use timeout-aware versions.
• dm/pkg/conn/basedb.go
  • Added GetDownstreamDBWithTimeout and getDBWithTimeout functions to support configurable timeouts for database connection retrieval.
  • Modified existing GetUpstreamDB and GetDownstreamDB functions to utilize the new timeout-aware connection logic.
  • Extended DefaultDBProviderImpl with ApplyWithPingTimeout to allow custom ping timeouts during database connection establishment.
• dm/tests/openapi/run.sh
  • Adjusted test_delete_task_with_stopped_downstream to reflect that task deletion now succeeds even if the downstream is stopped.
  • Added a new test test_delete_task_with_downstream_meta_cleanup_error to validate behavior when downstream metadata cleanup fails.
  • Updated the main run function to include the new test case.

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

[gemini-code-assist]

Code Review

The pull request refactors the deleteTask functionality in the DM master to improve metadata cleanup, specifically by separating internal and downstream metadata removal and introducing configurable timeouts for database operations. This allows for more resilient task deletion, where errors during downstream metadata cleanup are now logged as warnings instead of blocking the entire deletion process. New helper functions like AdjustTargetDBSessionCfgWithTimeout, removeMetaDataWithTimeout, removeInternalMetaData, removeDownstreamMetaData, GetDownstreamDBWithTimeout, and ApplyWithPingTimeout were introduced or modified to support these changes, along with updated and new test cases. Review comments suggest improvements to error handling, specifically by ensuring that removeInternalMetaData does not mask errors by returning nil and by addressing the incomplete error handling that could skip downstream metadata removal if internal metadata removal fails. Additionally, a code simplification was suggested for a redundant else if block in the deleteTask function.

[gemini-code-assist]

Returning nil here after encountering an error in s.optimist.RemoveMetaDataWithTask(taskName) or s.scheduler.RemoveLoadTaskAndLightningStatus(taskName) might mask issues. It would be better to return the error to ensure it's handled appropriately by the caller.

		return err
		}
		return nil //nolint:nilerr
	}

	return err // Return the error instead of nil

[gemini-code-assist]

The else if block here is redundant. If the adjustErr check fails, the function continues, so the else is unnecessary. The code can be simplified by removing the else and letting the subsequent code execute unconditionally.

		log.L().Warn("skip downstream metadata cleanup when deleting task", zap.String("task", taskName), zap.Error(adjustErr))
		 if err = s.removeDownstreamMetaData(ctx, taskName, metaSchema, toDBCfg, openAPIDeleteTaskDownstreamTimeout); err != nil {
			log.L().Warn("failed to remove downstream metadata when deleting task", zap.String("task", taskName), zap.Error(err))

[gemini-code-assist]

This error handling seems incomplete. If s.removeInternalMetaData fails, the function returns, skipping the downstream metadata removal. It might be better to log this error and continue with downstream metadata removal, or to attempt downstream removal before internal metadata removal.

	return err
		}

		// Log the error but continue with downstream metadata removal
		log.L().Error("failed to remove internal metadata", zap.String("task", taskName), zap.Error(err))
	}