Skip to content

Conversation

@zimulala
Copy link
Contributor

@zimulala zimulala commented Jul 7, 2025

This refactor focuses on improving the readability and structural clarity of the internal/client/client.go and client_batch.go files. Key changes include:

  • Splitting large files and isolating batch-related logic into more focused structures.

    • Moved batchConn from internal/client/client_batch.go to internal/client/conn_batch.go
    • Moved connArray from internal/client/client.go to internal/client/conn_pool.go
    • Moved monitoredConn and connMonitor from internal/client/client.gotointernal/client/conn_monitor.go`
  • Renaming unclear struct fields, such as connArray.vconns, and RPCClient.connsconnPools, for better semantic clarity.

    截屏2025-07-08 13 21 40

…ural clarity

Signed-off-by: Lynn <zimu_xia@126.com>
@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 7, 2025
zimulala added 2 commits July 7, 2025 17:32
Signed-off-by: Lynn <zimu_xia@126.com>
Signed-off-by: Lynn <zimu_xia@126.com>
@zimulala zimulala force-pushed the zimuxia/update-conn-pool branch from 6594135 to cbe4e55 Compare July 7, 2025 09:50
@MyonKeminta MyonKeminta requested a review from zyguan July 8, 2025 03:08
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

I'm ok to the changes, but I think maybe we need @zyguan to give a convincing opinion about whether the refactor is proper


// batchCommandsCh used for batch commands.
batchCommandsCh chan *batchCommandsEntry
batchCommandsConns []*batchCommandsClient
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks not proper to be renamed ...Conns. It's not a real connection, and the word client comes from the naming of goproto. It's the client of the stream established by calling gRPC stream API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this batchConn is not particularly reasonable; it is embedded. I just want to say, with the conns field analogy, and then change batchCommandsClients to batchCommandsConns. Mostly, I didn't figure out what would be better to change batchConn to.
Let me just change the batchCommandsClients back.

Signed-off-by: Lynn <zimu_xia@126.com>
}

// ForceRefreshAllStores get all stores from PD and refresh store cache.
func (c *RegionCache) ForceRefreshAllStores(ctx context.Context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is #1686 deprecated?

Copy link
Contributor Author

@zimulala zimulala Jul 9, 2025

Choose a reason for hiding this comment

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

Yes. @guo-shaoge added it. Recently, he found that it was not used, so he hoped that I could help delete it.

Signed-off-by: Lynn <zimu_xia@126.com>
@zimulala
Copy link
Contributor Author

zimulala commented Jul 9, 2025

PTAL @zyguan

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 9, 2025
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 9, 2025
@ti-chi-bot ti-chi-bot bot removed the lgtm label Jul 9, 2025
@zyguan
Copy link
Contributor

zyguan commented Jul 9, 2025

/retest

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 9, 2025
@zyguan
Copy link
Contributor

zyguan commented Jul 9, 2025

/retest

@ti-chi-bot ti-chi-bot bot added the lgtm label Jul 10, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 10, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MyonKeminta, zyguan

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

The pull request process is described here

Details 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 Jul 10, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 10, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-07-09 04:55:03.087855991 +0000 UTC m=+2061955.811034958: ☑️ agreed by zyguan.
  • 2025-07-09 07:45:37.349558757 +0000 UTC m=+2072190.072737735: ☑️ agreed by MyonKeminta.
  • 2025-07-09 07:48:52.862671887 +0000 UTC m=+2072385.585850880: ✖️🔁 reset by zimulala.
  • 2025-07-09 10:33:26.439983735 +0000 UTC m=+2082259.163162716: ☑️ agreed by zyguan.
  • 2025-07-10 04:15:39.381243115 +0000 UTC m=+2145992.104422097: ☑️ agreed by MyonKeminta.

@ti-chi-bot ti-chi-bot bot merged commit e60fec1 into tikv:master Jul 10, 2025
17 of 18 checks passed
@zimulala zimulala deleted the zimuxia/update-conn-pool branch July 10, 2025 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants