<fix>[testlib]: Bump servlet to 3.1 #3130
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves: ZSTAC-12345 Change-Id: I76726765796c6b7167617a666c68797668736a75
MatheMatrix
force-pushed
the
sync/ye.zou/upgrade-servlet@@2
branch
from
a15c0e6 to
6e765ef
Compare
Walkthrough将消息超时配置提取到新工具类 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as TestLibController
participant Config as TestConfigUtils
participant Pool as ThreadPool
participant Handler as Test.handleHttp
Client->>Controller: 发起 HTTP 请求
Controller->>Config: getMessageTimeoutMillsConfig()
Config-->>Controller: 超时(ms)
Controller->>Controller: startAsync() 并设置 timeout
Controller->>Pool: submit 工作任务
Pool->>Handler: 调用 handleHttp(request,response)
Handler-->>Pool: 处理完成 / 抛出异常
Pool->>Controller: asyncContext.complete()
Controller-->>Client: 返回 HTTP 响应
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 Poem
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.*⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-01T08:16:10.006ZApplied to files:
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
testlib/src/main/java/org/zstack/testlib/util/TestConfigUtils.groovy (1)
5-21: 建议添加 JavaDoc 注释。工具类及其公共方法应该添加 JavaDoc 注释，说明用途、参数含义和返回值，提高代码可读性。
🔎 建议添加 JavaDoc
+/** + * Utility class for test configuration management. + */ class TestConfigUtils { private final static long DEFAULT_MESSAGE_TIMEOUT_SECS = TimeUnit.SECONDS.toMillis(25) + /** + * Gets the message timeout configuration in milliseconds. + * + * @return timeout in milliseconds. Returns 30 minutes if maven.surefire.debug is set, + * otherwise returns the value from msgTimeoutMins system property (in minutes converted to millis), + * or the default 25 seconds if not configured. + */ static long getMessageTimeoutMillsConfig(){testlib/src/main/java/org/zstack/testlib/TestLibController.java (1)
25-25: 建议使线程池大小可配置。线程池大小硬编码为 32 可能不适合所有测试场景。建议从配置中读取，提高灵活性。
🔎 可选的配置化改进
在
TestConfigUtils中添加配置方法：public static int getThreadPoolSize() { String poolSize = System.getProperty("testlib.threadpool.size"); return poolSize != null ? Integer.parseInt(poolSize) : 32; }然后在
TestLibController中使用：- private static final ExecutorService pool = Executors.newFixedThreadPool(32); + private static final ExecutorService pool = Executors.newFixedThreadPool(TestConfigUtils.getThreadPoolSize());
📜 Review details
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
conf/web.xmlis excluded by!**/*.xmlcore/pom.xmlis excluded by!**/*.xmlheader/pom.xmlis excluded by!**/*.xmlpom.xmlis excluded by!**/*.xmlportal/pom.xmlis excluded by!**/*.xmlrest/pom.xmlis excluded by!**/*.xmlsdk/pom.xmlis excluded by!sdk/**,!**/*.xmlsearch/pom.xmlis excluded by!**/*.xmlsimulator/pom.xmlis excluded by!**/*.xmltestlib/pom.xmlis excluded by!**/*.xmlutils/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (3)
testlib/src/main/java/org/zstack/testlib/Test.groovytestlib/src/main/java/org/zstack/testlib/TestLibController.javatestlib/src/main/java/org/zstack/testlib/util/TestConfigUtils.groovy
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写
Files:
testlib/src/main/java/org/zstack/testlib/util/TestConfigUtils.groovytestlib/src/main/java/org/zstack/testlib/Test.groovytestlib/src/main/java/org/zstack/testlib/TestLibController.java
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
- 避免使用魔法值（Magic Value）：
直接使用未经定义的数值或字符串（如 if (status == 5)）应替换为枚举或常量。
示例：
// 错误示例：魔法值
if (user.getStatus() == 5) { ... }
// 正确示例：常量或枚举
public static final int STATUS_ACTIVE = 5;
if (user.getStatus() == STATUS_ACTIVE) { ... }
// 或使用枚举
enum UserStatus { ACTIVE, INACTIVE }
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
...
Files:
testlib/src/main/java/org/zstack/testlib/TestLibController.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: zstack-robot-1
Repo: MatheMatrix/zstack PR: 2360
File: network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java:449-490
Timestamp: 2025-08-04T04:48:19.103Z
Learning: ZStack项目在cherry-pick操作中，即使发现了性能优化机会（如IP地址批量保存的内存优化），也严格遵循不做额外修改的政策，优先保证cherry-pick的完整性和一致性。
📚 Learning: 2025-09-01T08:16:10.006Z
Learnt from: MatheMatrix
Repo: MatheMatrix/zstack PR: 2541
File: testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy:30889-30893
Timestamp: 2025-09-01T08:16:10.006Z
Learning: ApiHelper.groovy in testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy is auto-generated and should not be manually modified or receive code change suggestions.
Applied to files:
testlib/src/main/java/org/zstack/testlib/Test.groovy
🔇 Additional comments (2)
testlib/src/main/java/org/zstack/testlib/Test.groovy (1)
28-28: 重构到工具类的做法很好。将消息超时配置逻辑提取到
TestConfigUtils工具类是一个良好的实践，符合单一职责原则，提高了代码的可维护性。Also applies to: 306-307
testlib/src/main/java/org/zstack/testlib/TestLibController.java (1)
40-57: 异步处理实现正确。使用 Servlet 3.1 的
AsyncContext实现异步请求处理，配合线程池执行，符合升级到 Servlet 3.1 的目标。异常处理和资源清理（asyncContext.complete()）都处理得当。
| public class TestLibController { | ||
| private static final CLogger logger = Utils.getLogger(TestLibController.class); | ||
|
|
||
| private static final ExecutorService pool = Executors.newFixedThreadPool(32); |
There was a problem hiding this comment.
线程池未关闭可能导致资源泄漏。
ExecutorService 在应用生命周期内创建但从未关闭。虽然在测试环境中可能不是严重问题，但在生产环境或长时间运行的测试套件中可能导致线程泄漏。建议添加关闭机制，例如使用 Spring 的 @PreDestroy 注解或在适当的清理阶段关闭线程池。
🔎 建议添加线程池关闭机制
@Controller
public class TestLibController {
private static final CLogger logger = Utils.getLogger(TestLibController.class);
private static final ExecutorService pool = Executors.newFixedThreadPool(32);
+
+ @PreDestroy
+ public void destroy() {
+ pool.shutdown();
+ try {
+ if (!pool.awaitTermination(5, TimeUnit.SECONDS)) {
+ pool.shutdownNow();
+ }
+ } catch (InterruptedException e) {
+ pool.shutdownNow();
+ Thread.currentThread().interrupt();
+ }
+ }需要在文件开头添加导入：
import javax.annotation.PreDestroy;
import java.util.concurrent.TimeUnit;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static final ExecutorService pool = Executors.newFixedThreadPool(32); | |
| @Controller | |
| public class TestLibController { | |
| private static final CLogger logger = Utils.getLogger(TestLibController.class); | |
| private static final ExecutorService pool = Executors.newFixedThreadPool(32); | |
| @PreDestroy | |
| public void destroy() { | |
| pool.shutdown(); | |
| try { | |
| if (!pool.awaitTermination(5, TimeUnit.SECONDS)) { | |
| pool.shutdownNow(); | |
| } | |
| } catch (InterruptedException e) { | |
| pool.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In testlib/src/main/java/org/zstack/testlib/TestLibController.java around line
25 the static ExecutorService pool is created but never closed, risking
thread/resource leaks; add a lifecycle/cleanup method (e.g., annotate a public
void shutdown() with @PreDestroy) that calls pool.shutdown(), awaits termination
with a reasonable timeout using TimeUnit (and if necessary calls shutdownNow()
on timeout), and log or handle interrupted exceptions; also add the imports
javax.annotation.PreDestroy and java.util.concurrent.TimeUnit.
| class TestConfigUtils { | ||
| private final static long DEFAULT_MESSAGE_TIMEOUT_SECS = TimeUnit.SECONDS.toMillis(25) | ||
|
|
||
| static long getMessageTimeoutMillsConfig(){ |
There was a problem hiding this comment.
方法名拼写错误。
方法名 getMessageTimeoutMillsConfig 中的 "Mills" 应为 "Millis"（毫秒的正确缩写）。
🔎 建议修正拼写错误
- static long getMessageTimeoutMillsConfig(){
+ static long getMessageTimeoutMillisConfig(){注意：如果修改方法名，需要同步更新 Test.groovy 和 TestLibController.java 中的调用。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static long getMessageTimeoutMillsConfig(){ | |
| static long getMessageTimeoutMillisConfig(){ |
🤖 Prompt for AI Agents
In testlib/src/main/java/org/zstack/testlib/util/TestConfigUtils.groovy around
line 8, the method name getMessageTimeoutMillsConfig is misspelled — "Mills"
should be "Millis"; rename the method to getMessageTimeoutMillisConfig and
update all call sites (including Test.groovy and TestLibController.java) to use
the new name, then run a build to verify no remaining references.
Comment on lines
+19
to
+20
| long msgTimeout = Long.parseLong(msgTimeoutStr) | ||
| return TimeUnit.MINUTES.toMillis(msgTimeout) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
缺少异常处理。
Long.parseLong(msgTimeoutStr) 在接收到无效输入时会抛出 NumberFormatException。建议添加 try-catch 处理，并在解析失败时返回默认值或记录警告日志。
🔎 建议添加异常处理
+ try {
long msgTimeout = Long.parseLong(msgTimeoutStr)
return TimeUnit.MINUTES.toMillis(msgTimeout)
+ } catch (NumberFormatException e) {
+ // Log warning and return default value
+ return DEFAULT_MESSAGE_TIMEOUT_SECS
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In testlib/src/main/java/org/zstack/testlib/util/TestConfigUtils.groovy around
lines 19-20, Long.parseLong(msgTimeoutStr) can throw NumberFormatException for
invalid input; wrap the parse in a try-catch, catch NumberFormatException (and
any NPE if needed), log a warning with the offending value and exception, and
return a sensible default timeout (e.g., previously expected default) when
parsing fails so the method never propagates the exception.
Resolves: ZSTAC-12345 Change-Id: I626979756873727a637676676b75786e74636a6d
MatheMatrix
force-pushed
the
sync/ye.zou/upgrade-servlet@@2
branch
from
6e765ef to
3144aaa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: ZSTAC-12345
Change-Id: I76726765796c6b7167617a666c68797668736a75
sync from gitlab !8943