Repository navigation
planner: cache mpp info to avoid frequent grpc calling (#65694) - #69403
ti-chi-bot[bot] merged 2 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@xzhangxian1008 This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughIntroduces a global LRU cache ( ChangesTiFlash Server Info Caching
Sequence DiagramsequenceDiagram
participant Optimizer
participant splitTiFlashLogicalCoreCache
participant GlobalMPPServerInfoManager
participant FetchCluster as FetchClusterServerInfoWithoutPrivilegeCheck
participant Goroutines
Optimizer->>splitTiFlashLogicalCoreCache: tiflashServersInfo
splitTiFlashLogicalCoreCache->>GlobalMPPServerInfoManager: Get(address) per server
GlobalMPPServerInfoManager-->>splitTiFlashLogicalCoreCache: MPPServerInfo or nil
splitTiFlashLogicalCoreCache-->>Optimizer: serversNeedingRefresh, minLogicalCores
alt serversNeedingRefresh not empty
Optimizer->>FetchCluster: serversNeedingRefresh, HardwareInfo
FetchCluster->>Goroutines: spawn per-server goroutines
Goroutines-->>FetchCluster: ServerInfoResult{Idx, Rows or Err}
FetchCluster-->>Optimizer: []ServerInfoResult (sorted by Idx)
Optimizer->>GlobalMPPServerInfoManager: Add(MPPServerInfo) per refreshed server
Optimizer->>Optimizer: recompute minLogicalCores
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/infoschema/tables.go (2)
2628-2633: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
Idxinput-order contract.
Idxis now used by callers to map rows back to the correspondingserversInfoentry. Please make that invariant explicit on the exported type/field.Proposed documentation tweak
-// ServerInfoResult contains server info results +// ServerInfoResult contains the fetch result for one input ServerInfo. type ServerInfoResult struct { - Idx int + // Idx is the index of the corresponding ServerInfo in the input slice. + Idx int Rows [][]types.Datum Err error }As per coding guidelines, comments should explain non-obvious intent, constraints, invariants, and exported-symbol doc comments should prefer semantic constraints over name restatement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/infoschema/tables.go` around lines 2628 - 2633, The `Idx` field in the `ServerInfoResult` struct is missing documentation about an important invariant that callers depend on. Add a comment above the `Idx` field in the `ServerInfoResult` struct that explicitly documents that this field is used by callers to map the returned rows back to the corresponding entry in the original `serversInfo` input, making this contract and constraint clear to anyone using this exported type.Source: Coding guidelines
2665-2670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the failed server in the warning log.
When
recordWarningInStmtCtxis false, the log drops which server failed even thoughresult.Idxcan identify it. Add address/type fields so the gRPC failure is actionable.As per coding guidelines, error handling should be actionable and contextual.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/infoschema/tables.go` around lines 2665 - 2670, When recordWarningInStmtCtx is false, the log.Warn call does not include which server failed, making the gRPC failure non-actionable. Enhance the log.Warn call in the else branch to include the server address and type information that can be identified using result.Idx, so that the error log message contains contextual details about which server caused the failure alongside the error message from result.Err.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/optimizer.go`:
- Around line 51-56: The file contains unresolved merge conflict markers in two
locations that must be resolved before the code can compile. In the import block
at the top of the file, remove the merge conflict markers (<<<<<<< HEAD,
=======, and >>>>>>> 84f3ca52e57) and decide which imports are needed: keep the
variable import from HEAD and also include the vardef and copr imports from the
incoming branch if both are required by the code. Similarly, locate and resolve
the merge conflict in the getTiFlashServerMinLogicalCores function (around line
687-721) by removing the conflict markers and determining which version of the
function implementation is correct based on the cache MPP info feature being
implemented. Ensure all merge conflict markers are completely removed and the
resulting imports and function implementation are syntactically valid Go code.
In `@pkg/store/copr/BUILD.bazel`:
- Around line 92-96: Resolve the merge conflict in the BUILD.bazel file at the
shard_count declaration. Remove all merge conflict markers (the lines starting
with <<<<<<<, =======, and >>>>>>>) and replace them with a single shard_count
assignment. Choose the appropriate value for the release-8.5 branch (either 33
from HEAD or 43 from the incoming commit 84f3ca52e57) based on your branch's
requirements, then delete all marker lines so Bazel can properly parse the
target definition.
---
Nitpick comments:
In `@pkg/infoschema/tables.go`:
- Around line 2628-2633: The `Idx` field in the `ServerInfoResult` struct is
missing documentation about an important invariant that callers depend on. Add a
comment above the `Idx` field in the `ServerInfoResult` struct that explicitly
documents that this field is used by callers to map the returned rows back to
the corresponding entry in the original `serversInfo` input, making this
contract and constraint clear to anyone using this exported type.
- Around line 2665-2670: When recordWarningInStmtCtx is false, the log.Warn call
does not include which server failed, making the gRPC failure non-actionable.
Enhance the log.Warn call in the else branch to include the server address and
type information that can be identified using result.Idx, so that the error log
message contains contextual details about which server caused the failure
alongside the error message from result.Err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b50a9495-2a7f-4c17-91dd-fe7b4ee3c735
📒 Files selected for processing (8)
pkg/executor/memtable_reader.gopkg/infoschema/tables.gopkg/planner/core/BUILD.bazelpkg/planner/core/optimizer.gopkg/planner/core/optimizer_test.gopkg/store/copr/BUILD.bazelpkg/store/copr/mpp_probe.gopkg/store/copr/mpp_probe_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #69403 +/- ##
================================================
Coverage ? 56.5944%
================================================
Files ? 1852
Lines ? 676824
Branches ? 0
================================================
Hits ? 383045
Misses ? 265818
Partials ? 27961
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
added
cherry-pick-approved
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: windtalker, winoros, wjhuang2016, xzhangxian1008 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
This is an automated cherry-pick of #65694
What problem does this PR solve?
Issue Number: close #65701
Problem Summary:
What changed and how does it work?
We define a new struct called
MPPInfoManagerand set a global variableGlobalMPPInfoManagerwithMPPInfoManagertype. mpp info are all cached in this variable. IngetTiFlashServerMinLogicalCores, we will check the cache first then call grpc for uncached tiflash stores. When we get a failed tiflash store infilterAliveStoresHelper, we will remove it's cache.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests