tiflow icon indicating copy to clipboard operation
tiflow copied to clipboard

.*: upgrade tidb and parser

Open GMHDBJD opened this issue 7 months ago • 5 comments

What problem does this PR solve?

Issue Number: close #xxx

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Questions

Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note.

If you don't think this PR needs a release note then fill it with `None`.

GMHDBJD avatar May 27 '25 12:05 GMHDBJD

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

:open_book: For more info, you can check the "Contribute Code" section in the development guide.

ti-chi-bot[bot] avatar May 27 '25 12:05 ti-chi-bot[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 benmeadowcroft, gmhdbjd 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.

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[bot] avatar May 27 '25 12:05 ti-chi-bot[bot]

@gemini-code-assist[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Code Review

This pull request primarily focuses on upgrading TiDB, its parser, and the PD client dependencies. The changes across most files are mechanical refactorings to adapt to API modifications in these upgraded libraries. Key areas of change include:

  1. PD Client API Usage: Updated calls to pd.NewClientWithContext to include caller.Component and use the opt package for options (e.g., opt.WithGRPCDialOptions, opt.WithForwardingOption). This is consistent across cdc/api/v2/api_helpers.go, cdc/server/server.go, pkg/cmd/factory/factory_impl.go, and pkg/migrate/migrate.go.
  2. TiDB Parser Model Changes: Numerous files have been updated to replace pmodel.CIStr (and pmodel.NewCIStr) with ast.CIStr (and ast.NewCIStr). This reflects changes in how TiDB's parser exposes case-insensitive strings and other model types (e.g., ast.PartitionTypeRange, ast.IndexTypeBtree). These changes are widespread and appear consistently applied.
  3. TiDB Session Context Variable Changes: variable.DefMaxAllowedPacket is now vardef.DefMaxAllowedPacket, and variable.ClusteredIndexDefModeOff is now vardef.ClusteredIndexDefModeOff, reflecting refactoring within TiDB's session context packages.
  4. Dependency Updates in go.mod / go.sum: These files reflect the upgraded versions of TiDB, PD client, and other related libraries. The Go version has also been updated, and a toolchain version specified.

Two specific adaptations stand out and are crucial for correctness with the new library versions:

  • In pkg/pdutil/api_client.go, the way member count is retrieved from GetAllMembers response has been updated.
  • In pkg/sink/codec/csv/csv_decoder.go, field names in lightning/config.CSVConfig have been updated.

Overall, the changes seem to correctly adapt the codebase to the new dependency versions. The PR description could be enhanced with more details about the upgrade process or any specific challenges encountered, but the code changes themselves are largely as expected for such an upgrade.

Summary of Findings

  • PD Client API Adaptation for Member Listing: In pkg/pdutil/api_client.go, the logic for collecting PD member endpoints was updated to use members.Members, reflecting a likely change in the GetAllMembers response structure from the PD client library. This is a critical adaptation for correct functionality.
  • TiDB Lightning CSVConfig API Adaptation: In pkg/sink/codec/csv/csv_decoder.go, field names for lightning/config.CSVConfig were updated (e.g., Separator to FieldsTerminatedBy). This is a necessary change to align with API updates in the TiDB Lightning library, ensuring correct CSV parsing configuration.
  • Widespread Parser Model Refactoring: Numerous files were updated to reflect changes in TiDB's parser, primarily replacing pmodel.CIStr with ast.CIStr and updating paths for other parser model types. These changes are consistent with a parser library upgrade.
  • PD Client Option Updates: Calls to pd.NewClientWithContext and related options were updated to use the new opt package and include caller.Component. This is standard for PD client upgrades.
  • Dependency Version Bumps: The go.mod and go.sum files show updates to TiDB, PD client, and various other dependencies, along with a Go version and toolchain update. These are the core of the upgrade.

Merge Readiness

The pull request appears to be a well-executed dependency upgrade, with most changes being necessary adaptations to upstream API changes in TiDB, its parser, and the PD client. The critical adaptations in pkg/pdutil/api_client.go and pkg/sink/codec/csv/csv_decoder.go seem correct and ensure compatibility with the new library versions.

Given the nature of these changes, the PR looks to be in good shape. However, as I am an AI, I am not authorized to approve pull requests. Please ensure thorough testing and review by other maintainers before merging.

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] avatar May 27 '25 12:05 ti-chi-bot[bot]

/retest

lance6716 avatar Jun 03 '25 09:06 lance6716

@GMHDBJD: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-verify 96fc83ec3ed670a227e13c768634070dc28f9826 link true /test pull-verify
pull-dm-integration-test 96fc83ec3ed670a227e13c768634070dc28f9826 link true /test pull-dm-integration-test

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.

ti-chi-bot[bot] avatar Jun 03 '25 09:06 ti-chi-bot[bot]