executor: skip analyze flush broadcast in test binaries#68417
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
@0xPoe I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/S
0xPoe
changed the title
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplace the ChangesAnalyze stats delta flushing topology detection
Sequence DiagramsequenceDiagram
participant Analyze
participant runningUnderGoTest
participant FlushAction
Analyze->>runningUnderGoTest: call testing.Testing()
runningUnderGoTest-->>Analyze: bool result
Analyze->>FlushAction: [true] local dump
Analyze->>FlushAction: [false] cluster FLUSH STATS_DELTA
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68417 +/- ##
================================================
- Coverage 77.2761% 75.6786% -1.5975%
================================================
Files 2010 2008 -2
Lines 555473 564521 +9048
================================================
- Hits 429248 427222 -2026
- Misses 125305 137195 +11890
+ Partials 920 104 -816
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
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/analyze_utils_test.go (1)
74-81: ⚡ Quick winAvoid replacing the entire global config in this test.
Using
config.NewConfig()andStoreGlobalConfig(...)here swaps all global config fields, which increases cross-test coupling risk. Prefer preserving the existing config and changing onlyAdvertiseAddressfor each case.As per coding guidelines, keep test changes minimal and deterministic; avoid broad churn unless required.
🤖 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/executor/analyze_utils_test.go` around lines 74 - 81, The test currently replaces the entire global config using config.NewConfig() + config.StoreGlobalConfig(...), which is risky; instead, read the existing global config, copy it and only change the AdvertiseAddress field for the two cases, call config.StoreGlobalConfig with that modified copy, and ensure you restore the original config with defer after the test; update the instances where AdvertiseAddress is set (referencing AdvertiseAddress, config.StoreGlobalConfig, and isInProcessTopology) so only AdvertiseAddress is mutated and the rest of the global config remains unchanged.
🤖 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.
Nitpick comments:
In `@pkg/executor/analyze_utils_test.go`:
- Around line 74-81: The test currently replaces the entire global config using
config.NewConfig() + config.StoreGlobalConfig(...), which is risky; instead,
read the existing global config, copy it and only change the AdvertiseAddress
field for the two cases, call config.StoreGlobalConfig with that modified copy,
and ensure you restore the original config with defer after the test; update the
instances where AdvertiseAddress is set (referencing AdvertiseAddress,
config.StoreGlobalConfig, and isInProcessTopology) so only AdvertiseAddress is
mutated and the rest of the global config remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a92f652-e0af-42b3-a403-759f857dd18b
📒 Files selected for processing (3)
pkg/executor/BUILD.bazelpkg/executor/analyze.gopkg/executor/analyze_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/executor/analyze.go
ti-chi-bot
Bot
added
size/S
|
Tested locally: Before
After |
0xPoe
changed the title
0xPoe
changed the title
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
`make bench-daily` (CI job `gobench4`) fails on `BenchmarkNonPartitionPointGetPlanCacheOn`: `analyze table t` hits `context deadline exceeded` after 10s. PR pingcap#67939 added `flushStatsDeltaForAnalyze`, which broadcasts FLUSH STATS_DELTA CLUSTER via a TiDB-type cop request before each ANALYZE. The local-dump fallback was gated on `intest.InTest`, but that variable is only set under the `intest` build tag. `make bench-daily` runs `go test` without the tag, so the broadcast hits the mockstore session's empty `:10080` and hangs. Replace the gate with a `runningUnderGoTest` helper backed by `testing.Testing()`, which the linker sets to true for any binary produced by `go test` (covering both unit tests and bench-daily) and leaves false for `go build` of cmd/tidb-server. The gate is therefore a no-op in production regardless of TOML host configuration, and no longer depends on the `intest` build tag. Issue Number: close pingcap#68416
0xPoe
left a comment
0xPoe
left a comment
There was a problem hiding this comment.
🔢 Self-check (PR reviewed by myself and ready for feedback)
-
Code compiles successfully
-
Unit tests added
-
No AI-generated elegant nonsense in PR.
-
Comments added where necessary
-
PR title and description updated
-
Documentation PR created (or confirmed not needed)
-
PR size is reasonable
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjonss, terry1purcell 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
0xPoe
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
|
Thanks for your review! 💚 💙 💜 💛 ❤️ |
4 participants
What problem does this PR solve?
Issue Number: close #68416
Problem Summary:
make bench-daily(CI jobgobench4) fails onBenchmarkNonPartitionPointGetPlanCacheOn:analyze table thitscontext deadline exceededafter 10s. PR #67939 addedflushStatsDeltaForAnalyze, which broadcastsFLUSH STATS_DELTA CLUSTERvia a TiDB-type cop request before each ANALYZE. The local-dump fallback was gated onintest.InTest, but that variable is only set under theintestbuild tag.make bench-dailyrunsgo testwithout the tag, so the broadcast hits the mockstore session's empty:10080and hangs.What changed and how does it work?
Replace the gate with a
runningUnderGoTesthelper backed bytesting.Testing(). The Go linker setstesting.Testing()totruefor any binary produced bygo test(covering both unit tests andmake bench-daily) and leaves itfalseforgo buildofcmd/tidb-server. The gate is therefore a no-op in production regardless of host or flag configuration, and no longer depends on theintestbuild tag.Check List
Tests
Reproduction and verification (same machine, same command):
#68417 (comment)
Side effects
Documentation
Release note
Summary by CodeRabbit