Unify branches 8.5 and master #3849
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[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 |
ti-chi-bot
bot
added
the
size/XXL
Summary of ChangesHello @tenfyzhong, 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 undertakes a significant refactoring effort to unify the codebase for compatibility with both classic and next-generation PD clients. By introducing new abstraction layers and leveraging Go build tags, the changes streamline PD client interactions and TiDB type handling. This ensures the system can seamlessly operate across different PD environments while maintaining backward compatibility and improving overall code maintainability. 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 is a significant refactoring to support both classic and next-gen PD clients using build tags. The changes are extensive and well-structured, introducing an abstraction layer (pkg/pdtype, etc.) to handle conditional compilation. The overall approach is sound and the implementation appears correct and consistent with the goal of unifying the branches. I have one suggestion for the Makefile to improve code clarity and maintainability.
|
/test all |
|
/test next-gen |
1. Use a dedicated go.mod for nextgen mode. 2. Use the 8.5 branch as the default dependency. Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Rename `UnifyGetServiceGCSafepoint` to `GetServiceGCSafepoint` - Rename `UnifyDeleteGcSafepoint` to `RemoveServiceGCSafepoint` - Update all callers to use new function names Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Extract TiDB-specific constants and functions from `common` package to new `tidbtype` package - Create separate files for classic and nextgen builds to handle different TiDB versions - Update all imports to use new package location for `JobTableID`, `JobHistoryID`, and `ValidateKeyspace` - Maintain backward compatibility through package restructuring Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Migrate from `github.com/pingcap/tidb/pkg/parser/ast` to internal `github.com/pingcap/ticdc/pkg/tidbtype` for CIStr - Replace `github.com/pingcap/tidb/pkg/sessionctx/vardef` with internal tidbtype for DefMaxAllowedPacket - Update PD client method signatures to use internal types - Add nextgen-specific initialization files to handle conditional builds - Fix test expectations for row key encoding changes Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Update expected value in TestRowKey to match new behavior - Add TODO comment for follow-up verification of expected value difference Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Correct package name from 'common' to 'simple' in simple codec test - Correct package name from 'common' to 'mysql' in MySQL sink test - Add missing import for kerneltype in all three test files Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Include `-modfile=nextgen.go.mod` in unit_test_in_verify_ci_next_gen rule - Include `-modfile=nextgen.go.mod` in unit_test_pkg_next_gen rule Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Split GetServiceDiscovery method into separate files for classic and nextgen builds - Use `!nextgen` build tag for classic PD client interface - Use `nextgen` build tag for new PD service discovery interface - Remove generic implementation from main test file to avoid compilation issues Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Sort imports to maintain consistent ordering across test files - Move tidbtype import before util import in multiple test files Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Add separate tidy checks for classic and nextgen go.mod files - Use `-modfile` flag to target specific module files - Improve error handling and output messages for both modules - Maintain backward compatibility with existing tidy workflow Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
tenfyzhong
force-pushed
the
merge-master
branch
from
99292aa to
f65fa25
Compare
- Temporarily rename *_nextgen.go and *_nextgen_test.go files to .bak extensions when tidying classic go.mod - Temporarily rename *_classic.go and *_classic_test.go files when tidying nextgen go.mod - Temporarily swap go.mod/go.sum with nextgen.go.mod/nextgen.go.sum when tidying nextgen modules - Restore all files to original names after tidying operations - This prevents go mod tidy from parsing files meant for the other module configuration Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Remove separate check for classic go.mod/go.sum files - Consolidate tidy check to include all go.mod and go.sum files - Update error message to point to `make tidy` command Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Promote github.com/pingcap/sysutil from indirect to direct dependency - Update both go.mod and nextgen.go.mod files - Remove sysutil from indirect dependencies section Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Extract file hiding/restoration logic into reusable functions - Add switch_to_nextgen and restore_classic functions for go.mod management - Implement cleanup trap to ensure proper file restoration on exit - Fix backtick escaping in error message - Improve script readability and maintainability Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Update column and index name initialization in test cases to use tidbtype.CIStr instead of ast.CIStr - Maintain test functionality while aligning with type usage patterns Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Merge separate `-modfile` flag into same line as other test flags in `unit_test_in_verify_ci_next_gen` target - Combine `-modfile` flag with coverage arguments in `unit_test_pkg_next_gen` target Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Change single quotes to double quotes in awk command to ensure proper shell interpretation - This resolves potential issues with command execution in make targets Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
ti-chi-bot
bot
removed
do-not-merge/needs-triage-completed
do-not-merge/needs-linked-issue
labels
- Remove redundant -modfile flag duplication in conditional branches - Add -modfile flag once after conditional logic for cleaner code Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Move parallel test flag `P` definition closer to its usage context - Refactor `TEST_FLAG` variable to include race detection and parallel execution parameters - Consolidate `GOTEST` command by using expanded `TEST_FLAG` variable - Increase test timeout from 300s to 1200s for unit tests - Simplify next-gen test targets by reusing existing ones with `NEXT_GEN=1` - Remove redundant test command duplication for next-gen configurations Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Changed gotestsum -p parameter from 3 to 8 for faster test execution Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
/test all |
|
/test next-gen |
|
/retest |
…lity - Update import from `github.com/pingcap/tidb/pkg/parser/ast` to `github.com/pingcap/ticdc/pkg/tidbtype` - Replace `ast.NewCIStr()` with `tidbtype.NewCIStr()` for creating case-insensitive strings Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
/test all |
|
/test next-gen |
- Change `--tags intest` to `-tags intest` for correct go test flag syntax - Maintain compatibility with NEXT_GEN build option Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
- Replace `pd.NewClientWithContext` with `pdtype.NewClientWithContext` in resolve lock test - Add conditional build tag support for nextgen in test runner script Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
/test all |
|
/hold |
ti-chi-bot
bot
added
the
do-not-merge/hold
|
/retest |
|
/test next-gen |
Add GO_RUN_TAGS environment variable to test_prepare that is set to '-tags=nextgen' when NEXT_GEN=1, otherwise empty. This allows all integration test scripts to use a single 'go run $GO_RUN_TAGS ...' command instead of duplicated if-else blocks for NEXT_GEN handling. This simplifies the test scripts and makes it easier to add new tests that need to support both classic and nextgen builds. Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
@tenfyzhong: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
1 participant
What problem does this PR solve?
Issue Number: close #3914
What is changed and how it works?
The changes in this PR primarily involve refactoring the PD client usage to support both the classic and next-gen PD clients through build tags.
Create a dedicated
go.modfor next-gen mode.Here's a breakdown of the key changes:
pkg/pdtype: This new package provides an abstraction layer for PD client functionalities. It defines interfaces and implementations for both classic and next-generation PD clients.!nextgenandnextgen) are used to conditionally compile the appropriate PD client implementation based on the build environment.pkg/pdtype. This ensures that the code remains consistent regardless of the PD client version.go.modfiles have been updated to include the necessary dependencies and build configurations for the next-generation PD client.pkg/txnutil/gcandpkg/commonto align with the new PD client abstraction. Specifically,SetServiceGCSafepoint,GetServiceGCSafepoint,RemoveServiceGCSafepoint, andKeyspaceOfStoragenow correctly handle keyspace information.logservice/schemastorehave been updated to use the newtidbtype.CIStrandtidbtype.NewCIStrfor table names, ensuring compatibility with the next-generation PD client.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No, this change is designed to be backward compatible and should not introduce performance regressions. The abstraction layer ensures that the behavior remains consistent with the classic PD client.
Do you need to update user documentation, design documentation or monitoring documentation?
No, user-facing documentation does not need to be updated as this is an internal refactoring. Design documentation may benefit from an update to reflect the new PD client abstraction.
Release note