cmd/cdc/server: return code 0 on graceful shutdown#4564
cmd/cdc/server: return code 0 on graceful shutdown#4564ti-chi-bot[bot] merged 6 commits intopingcap:masterfrom
Conversation
Signed-off-by: Ping Yu <yuping@pingcap.com>
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThe changes implement proper exit status handling for graceful server shutdown by introducing a function to classify shutdowns as "normal" (nil error or context canceled), decoupling the server close operation from the potentially canceled run context, and suppressing error returns for normal exits to ensure exit code 0 on graceful termination. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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 |
Summary of ChangesHello, 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 resolves an issue where the 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of the server exiting with a non-zero status code on graceful shutdown. The introduction of the isNormalServerShutdown helper function centralizes the logic for identifying a normal shutdown, making the code cleaner and easier to understand. The changes are well-tested with new unit tests covering various shutdown scenarios. I have one suggestion to make the implementation more idiomatic.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cdc/server/server.go`:
- Around line 160-167: The shutdown goroutine currently calls
svr.Close(context.Background()), which gives downstream Close calls no deadline;
change it to create a timeout context derived from GracefulShutdownTimeout
(e.g., ctx, cancel := context.WithTimeout(context.Background(),
GracefulShutdownTimeout)) and pass that ctx into svr.Close, ensuring you call
cancel() (defer or after Close) so resources are released; keep the existing
goroutine and select on ch/ticker as-is but ensure the timeout context is used
when invoking svr.Close to propagate the shutdown budget to all submodule Close
methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 637fb1d3-a012-4c6e-b3df-b51e2933861e
📒 Files selected for processing (2)
cmd/cdc/server/server.gocmd/cdc/server/server_test.go
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
cmd/cdc/server/server.go
Outdated
| defer ticker.Stop() | ||
| go func() { | ||
| svr.Close(ctx) | ||
| svr.Close(context.Background()) |
There was a problem hiding this comment.
I think the ctx parameter should be removed
|
/test all |
Signed-off-by: Ping Yu <yuping@pingcap.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/server.go (1)
472-473: Nested timeout is bounded by parent context.
timeoutCtxinherits fromcloseCtx, so the effective timeout for etcd cleanup ismin(cleanMetaDuration, remaining time in closeCtx). If module closures consume most of the 30-second budget, etcd cleanup may get less than its intended 10 seconds.This is acceptable behavior for bounding total shutdown time, but worth noting that
cleanMetaDurationis a soft limit rather than a guarantee.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/server.go` around lines 472 - 473, The current timeoutCtx is created from closeCtx (timeoutCtx, timeoutCancel := context.WithTimeout(closeCtx, cleanMetaDuration)), which makes its deadline the min of cleanMetaDuration and remaining closeCtx time; to guarantee etcd cleanup gets the full cleanMetaDuration regardless of how long prior module shutdown took, create the timeout from a fresh background context (use context.WithTimeout(context.Background(), cleanMetaDuration)) and use that for the etcd cleanup call(s) while still calling timeoutCancel; update references to timeoutCtx/timeoutCancel accordingly in the etcd cleanup path so it uses the independent deadline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/server.go`:
- Around line 472-473: The current timeoutCtx is created from closeCtx
(timeoutCtx, timeoutCancel := context.WithTimeout(closeCtx, cleanMetaDuration)),
which makes its deadline the min of cleanMetaDuration and remaining closeCtx
time; to guarantee etcd cleanup gets the full cleanMetaDuration regardless of
how long prior module shutdown took, create the timeout from a fresh background
context (use context.WithTimeout(context.Background(), cleanMetaDuration)) and
use that for the etcd cleanup call(s) while still calling timeoutCancel; update
references to timeoutCtx/timeoutCancel accordingly in the etcd cleanup path so
it uses the independent deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26ef4904-87a3-4d44-8bb7-dd63b5a5c5a8
📒 Files selected for processing (3)
cmd/cdc/server/server.gopkg/server/server.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cdc/server/server.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flowbehappy, wk989898 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 |
[LGTM Timeline notifier]Timeline:
|
close pingcap#4563 (cherry picked from commit f08ab83)
What problem does this PR solve?
Issue Number: close #4563
What is changed and how it works?
cmd/cdc/servercurrently logs a graceful shutdown as normal, but it stillreturns
context.Canceledfromsvr.Run. That makes the CLI exit with code1even when shutdown was triggered intentionally by
SIGTERM.This PR:
isNormalServerShutdownto classifynil,context.Canceled, andwrapped
context.Canceledas normal shutdown;options.run;nilfor normal shutdown socdc serverexits with code0;Check List
Tests
go test ./cmd/cdc/server/...Questions
Will it cause performance regression or break compatibility?
No. The change only affects error classification on the shutdown path and does
not change steady-state behavior or compatibility.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor