executor: keep analyze status remaining time in UTC#67229
Conversation
ti-chi-bot
Bot
added
the
release-note
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XXL
📝 WalkthroughWalkthroughKeep a UTC copy of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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 |
0xPoe
force-pushed
the
fix/show-analyze-status-remaining-timezone
branch
from
113c0ef to
c35c7a2
Compare
ti-chi-bot
Bot
added
size/M
0xPoe
changed the title
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67229 +/- ##
================================================
+ Coverage 77.7710% 77.7781% +0.0070%
================================================
Files 2022 1943 -79
Lines 554420 555803 +1383
================================================
+ Hits 431178 432293 +1115
- Misses 121500 123457 +1957
+ Partials 1742 53 -1689
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
0xPoe
changed the title
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
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.
-
Bazel files updated
-
Comments added where necessary
-
PR title and description updated
-
Documentation PR created (or confirmed not needed)
-
PR size is reasonable
| return nil, err | ||
| } | ||
| startTimeUTC = &t | ||
| startTime = types.NewTime(types.FromGoTime(t.In(sctx.GetSessionVars().TimeZone)), mysql.TypeDatetime, 0) |
There was a problem hiding this comment.
This is where the original bug comes form. We use the session time, but we applied it as UTC in later usage.
| ) (_ *time.Duration, percentage, totalCnt float64, err error) { | ||
| remainingDuration := time.Duration(0) | ||
| if startTime != nil { | ||
| start, err := startTime.GoTime(time.UTC) |
|
/retest |
Signed-off-by: 0xPoe <techregister@pm.me> fix Signed-off-by: 0xPoe <techregister@pm.me>
0xPoe
force-pushed
the
fix/show-analyze-status-remaining-timezone
branch
from
8c94fd9 to
cc47e0c
Compare
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
Signed-off-by: 0xPoe <techregister@pm.me>
![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/infoschema_reader.go (1)
2582-2586: Consider returning a warning instead of failing the entire query.For a running job, returning an error if
startTimeUTCis nil will cause the entireSHOW ANALYZE STATUSto fail. If there's any corrupted row inmysql.analyze_jobswith a null start_time for a running job, users won't be able to see any analyze status.Consider logging a warning and either skipping the row or showing it with a nil remaining duration instead:
♻️ Suggested alternative approach
if state == statistics.AnalyzeRunning && !strings.HasPrefix(jobInfo, "merge global stats") { - if startTimeUTC == nil { - return nil, errors.New("invalid start time") - } - remainingDuration, progress, estimatedRowCnt, remainDurationErr := getRemainDurationForAnalyzeStatusHelper(ctx, sctx, startTimeUTC, - dbName, tableName, partitionName, processedRows) + if startTimeUTC != nil { + remainingDuration, progress, estimatedRowCnt, remainDurationErr := getRemainDurationForAnalyzeStatusHelper(ctx, sctx, startTimeUTC, + dbName, tableName, partitionName, processedRows) + } else { + logutil.BgLogger().Warn("running analyze job has invalid start time", + zap.String("db", dbName), + zap.String("table", tableName)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/infoschema_reader.go` around lines 2582 - 2586, The code currently returns an error when startTimeUTC is nil for a running analyze job (inside the branch where state == statistics.AnalyzeRunning and !strings.HasPrefix(jobInfo, "merge global stats")), which fails SHOW ANALYZE STATUS; instead, replace the return nil, errors.New("invalid start time") with a warning log and either skip that row (continue the loop) or populate the remaining-duration-related outputs with nil/zero values and proceed without calling getRemainDurationForAnalyzeStatusHelper; specifically, check startTimeUTC, call the logger (e.g., log.Warn / logutil.BgLogger().Warn) referencing jobInfo and any job id/context, and then either continue or set remainingDuration=nil, progress=0, estimatedRowCnt=nil and remainDurationErr=nil before the code continues to build the result row.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/executor/infoschema_reader.go`:
- Around line 2582-2586: The code currently returns an error when startTimeUTC
is nil for a running analyze job (inside the branch where state ==
statistics.AnalyzeRunning and !strings.HasPrefix(jobInfo, "merge global
stats")), which fails SHOW ANALYZE STATUS; instead, replace the return nil,
errors.New("invalid start time") with a warning log and either skip that row
(continue the loop) or populate the remaining-duration-related outputs with
nil/zero values and proceed without calling
getRemainDurationForAnalyzeStatusHelper; specifically, check startTimeUTC, call
the logger (e.g., log.Warn / logutil.BgLogger().Warn) referencing jobInfo and
any job id/context, and then either continue or set remainingDuration=nil,
progress=0, estimatedRowCnt=nil and remainDurationErr=nil before the code
continues to build the result row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8afa0903-63fc-48e3-a5fe-b2bbf33e575b
📒 Files selected for processing (1)
pkg/executor/infoschema_reader.go
Signed-off-by: 0xPoe <techregister@pm.me>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjonss, winoros 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/infoschema_reader.go (1)
2583-2586: Consider a more descriptive error message.The error "invalid start time" is returned when
startTimeUTCis nil for a running job, but it lacks context about which job or table is affected. Consider including identifiers for easier debugging.💡 Suggested improvement
if startTimeUTC == nil { - return nil, errors.New("invalid start time") + return nil, errors.Errorf("invalid start time for running analyze job on %s.%s", dbName, tableName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/infoschema_reader.go` around lines 2583 - 2586, The current nil check for startTimeUTC returns a generic error ("invalid start time"); update the check in infoschema_reader.go so the error includes contextual identifiers (e.g., job ID, table name, or analyze job info available in the surrounding scope) and any relevant state so callers can identify which running job failed; locate the check where startTimeUTC is validated before calling getRemainDurationForAnalyzeStatusHelper and replace the plain error with a formatted message that embeds those identifiers and the fact startTimeUTC was nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/executor/infoschema_reader.go`:
- Around line 2583-2586: The current nil check for startTimeUTC returns a
generic error ("invalid start time"); update the check in infoschema_reader.go
so the error includes contextual identifiers (e.g., job ID, table name, or
analyze job info available in the surrounding scope) and any relevant state so
callers can identify which running job failed; locate the check where
startTimeUTC is validated before calling getRemainDurationForAnalyzeStatusHelper
and replace the plain error with a formatted message that embeds those
identifiers and the fact startTimeUTC was nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63caa052-c79a-44b1-bfc8-1d8cc986ebd1
📒 Files selected for processing (1)
pkg/executor/infoschema_reader.go
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/test all |
0xPoe
added
the
needs-cherry-pick-release-8.5
|
/retest |
1 similar comment
|
/retest |
|
@0xPoe: 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. |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
4 participants
What problem does this PR solve?
Issue Number: close #67230
Problem Summary:
SHOW ANALYZE STATUScan report a negativeRemaining_secondswhen the sessiontime_zoneis ahead of UTC.What changed and how does it work?
Keep the UTC
start_timefor remaining-time calculation and continue using the session-local time only for display. This PR also adds a regression test that reproduces the negative duration under+08:00.Check List
Tests
Validation commands:
make failpoint-enablego test -run '^TestShowAnalyzeStatus$' -tags=intest,deadlock ./pkg/executormake failpoint-disablemake lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests