fix(http): avoid marking nodes down on client errors#360
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces a typed error for client-side HTTP failures (400/401/404/405) to prevent unnecessary server downtime marking. The ChangesClient Request Error Differentiation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
|
感谢您提出Pull Request,我会尽快Review。我会在1-2日内进行查看或者回复,如果遇到节假日可能会处理较慢,敬请谅解。 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf289294da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case http.StatusBadRequest, http.StatusUnauthorized, http.StatusNotFound, http.StatusMethodNotAllowed: | ||
| log.Errorf("Connect Apollo Server Fail, url:%s, StatusCode:%d", requestURL, res.StatusCode) | ||
| return nil, errors.New(fmt.Sprintf("Connect Apollo Server Fail, StatusCode:%d", res.StatusCode)) | ||
| return nil, &clientRequestInvalidError{statusCode: res.StatusCode} |
There was a problem hiding this comment.
Treat forbidden responses as client errors
When Apollo returns another client-side status such as 403 Forbidden for an unauthorized app/namespace, this still falls through to the retrying default case and RequestRecovery will call SetDownNode, marking a healthy node down for a bad request/permission issue. Apollo documents 403 as a client authorization error alongside 400/401/404/405 (https://github.com/apolloconfig/apollo/wiki/Apollo%E5%BC%80%E6%94%BE%E5%B9%B3%E5%8F%B0#%E5%9B%9B%E9%94%99%E8%AF%AF%E7%A0%81%E8%AF%B4%E6%98%8E), so this fix should classify the full non-retryable 4xx range (or at least include 403) before marking nodes down.
Useful? React with 👍 / 👎.
Summary
Why
Summary by CodeRabbit
Bug Fixes
Tests