feat: 优化代码[修改虚拟线程乱用或不合理的地方] #3242
Merged
Conversation
This file contains 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
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a series of significant architectural and code refactoring changes across multiple modules of the Laokou platform. The primary modifications include:
Changes
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (66)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
审阅者指南 by Sourcery这个拉取请求通过删除不必要或误用的虚拟线程来优化代码库。它修改了多个文件以实现这一目标，主要是将为单个任务创建的虚拟线程池替换为共享的虚拟线程池。 使用共享执行器的批处理优化序列图sequenceDiagram
participant Client
participant BatchProcessor
participant SharedExecutor
participant Database
Client->>BatchProcessor: batch(dataList)
activate BatchProcessor
Note over BatchProcessor: 将数据分区为块
BatchProcessor->>SharedExecutor: invokeAll(futures)
activate SharedExecutor
loop 对于每个块
SharedExecutor->>Database: 处理块
Database-->>SharedExecutor: 结果
end
SharedExecutor-->>BatchProcessor: 所有结果
deactivate SharedExecutor
BatchProcessor-->>Client: 完成
deactivate BatchProcessor
显示更新的映射器层次结构的类图classDiagram
class CrudMapper {
<<interface>>
+selectObjectCount(PageQuery)
+selectObjectPage(PageQuery)
}
class BaseMapper {
<<interface>>
}
class CustomMapper {
<<interface>>
}
CrudMapper --|> BaseMapper
CustomMapper --|> CrudMapper
note for CrudMapper "标准化的页面查询方法"
文件级别变更
Original review guide in EnglishReviewer's Guide by SourceryThis pull request optimizes the codebase by removing unnecessary or misused virtual threads. It modifies several files to achieve this, primarily by replacing virtual thread pools created for single tasks with a shared virtual thread pool. Sequence diagram for optimized batch processing with shared executorsequenceDiagram
participant Client
participant BatchProcessor
participant SharedExecutor
participant Database
Client->>BatchProcessor: batch(dataList)
activate BatchProcessor
Note over BatchProcessor: Partition data into chunks
BatchProcessor->>SharedExecutor: invokeAll(futures)
activate SharedExecutor
loop For each chunk
SharedExecutor->>Database: Process chunk
Database-->>SharedExecutor: Result
end
SharedExecutor-->>BatchProcessor: All results
deactivate SharedExecutor
BatchProcessor-->>Client: Complete
deactivate BatchProcessor
Class diagram showing updated mapper hierarchyclassDiagram
class CrudMapper {
<<interface>>
+selectObjectCount(PageQuery)
+selectObjectPage(PageQuery)
}
class BaseMapper {
<<interface>>
}
class CustomMapper {
<<interface>>
}
CrudMapper --|> BaseMapper
CustomMapper --|> CrudMapper
note for CrudMapper "Standardized page query methods"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿 @KouShenhai - 我已经审查了你的更改 - 以下是一些反馈：
整体评论：
- 对于简单的数据库分页查询，考虑移除虚拟线程的使用 - 异步复杂性对于快速操作（仅仅获取列表和计数）是不必要的。保留虚拟线程用于IO密集型操作，如文件处理和WebSocket。
以下是我在审查期间查看的内容
- 🟡 一般性问题：发现3个问题
- 🟢 安全性：一切看起来都很好
- 🟢 测试：一切看起来都很好
- 🟢 复杂性：一切看起来都很好
- 🟢 文档：一切看起来都很好
帮助我变得更有用！请在每条评论上点击 👍 或 👎，我将使用反馈来改进你的评论。
Original comment in English
Hey @KouShenhai - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing virtual thread usage for simple database pagination queries - the async complexity isn't needed for quick operations that just fetch a list and count. Keep virtual threads for IO-heavy operations like file handling and websockets.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -50,6 +50,8 @@ public class MybatisUtil { | |||
|
|
|||
| private static final int DEFAULT_BATCH_NUM = 100000; | |||
|
|
|||
| private static final ExecutorService EXECUTOR = ThreadUtil.newVirtualTaskExecutor(); | |||
There was a problem hiding this comment.
建议（性能）： 考虑添加适当的执行器关闭处理以防止资源泄漏
静态执行器需要仔细的生命周期管理。考虑实现关闭钩子或应用程序生命周期监听器，以确保执行器服务的正确清理。
Original comment in English
suggestion (performance): Consider adding proper executor shutdown handling to prevent resource leaks
Static executors need careful lifecycle management. Consider implementing a shutdown hook or application lifecycle listener to ensure proper cleanup of the executor service.
| c2.get(30, TimeUnit.SECONDS))); | ||
| try { | ||
| DynamicDataSourceContextHolder.push("domain"); | ||
| List<LoginLogDO> list = loginLogMapper.selectObjectPage(qry); |
There was a problem hiding this comment.
建议（性能）： 考虑保留数据库查询的超时保护
虽然代码现在更简单，但移除超时处理可能导致长时间运行的查询影响系统性能。考虑添加数据库查询超时配置。
建议的实现：
try {
DynamicDataSourceContextHolder.push("domain");
CompletableFuture<List<LoginLogDO>> listFuture = CompletableFuture.supplyAsync(() ->
loginLogMapper.selectObjectPage(qry));
CompletableFuture<Long> countFuture = CompletableFuture.supplyAsync(() ->
loginLogMapper.selectObjectCount(qry));
List<LoginLogDO> list = listFuture.get(30, TimeUnit.SECONDS);
long total = countFuture.get(30, TimeUnit.SECONDS);
return Result.ok(Page.create(list.stream().map(LoginLogConvertor::toClientObject).toList(), total));你需要在文件顶部添加这些导入：
- import java.util.concurrent.CompletableFuture;
- import java.util.concurrent.TimeUnit;
- import java.util.concurrent.TimeoutException;
你可能还想为TimeoutException添加错误处理，并将其包装在适当的特定于应用程序的异常中。
考虑通过应用程序属性使超时持续时间（30秒）可配置。
Original comment in English
suggestion (performance): Consider retaining timeout protection for database queries
While the code is simpler now, removing the timeout handling could lead to long-running queries impacting system performance. Consider adding a database query timeout configuration.
Suggested implementation:
try {
DynamicDataSourceContextHolder.push("domain");
CompletableFuture<List<LoginLogDO>> listFuture = CompletableFuture.supplyAsync(() ->
loginLogMapper.selectObjectPage(qry));
CompletableFuture<Long> countFuture = CompletableFuture.supplyAsync(() ->
loginLogMapper.selectObjectCount(qry));
List<LoginLogDO> list = listFuture.get(30, TimeUnit.SECONDS);
long total = countFuture.get(30, TimeUnit.SECONDS);
return Result.ok(Page.create(list.stream().map(LoginLogConvertor::toClientObject).toList(), total));You'll need to add these imports at the top of the file:
- import java.util.concurrent.CompletableFuture;
- import java.util.concurrent.TimeUnit;
- import java.util.concurrent.TimeoutException;
You may also want to add error handling for TimeoutException and wrap it in an appropriate application-specific exception.
Consider making the timeout duration (30 seconds) configurable via application properties.
| @@ -48,7 +48,7 @@ public class TraceLogConsumer { | |||
| @KafkaListener(topics = "laokou_trace_topic", groupId = "laokou_trace_consumer_group") | |||
| public CompletableFuture<Void> kafkaConsumer(List<String> messages, Acknowledgment ack) { | |||
| try { | |||
| Map<String, Object> dataMap = messages.stream() | |||
| Map<String, Object> dataMap = messages.parallelStream() | |||
There was a problem hiding this comment.
问题（性能）： 并行流处理需要背压处理
在没有背压机制的情况下使用parallelStream()可能在高负载下导致资源耗尽。考虑使用有界队列或速率限制器来控制处理。
Original comment in English
issue (performance): Parallel stream processing needs backpressure handling
Using parallelStream() without backpressure mechanisms could lead to resource exhaustion under high load. Consider using a bounded queue or rate limiter to control processing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3242 +/- ##
============================================
+ Coverage 24.29% 24.40% +0.10%
- Complexity 182 183 +1
============================================
Files 140 140
Lines 1922 1922
Branches 123 123
============================================
+ Hits 467 469 +2
+ Misses 1399 1397 -2
Partials 56 56 ☔ View full report in Codecov by Sentry. |
|
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.
Summary by Sourcery
增强功能：
Original summary in English
Summary by Sourcery
Enhancements:
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Code Refactoring
Performance Optimization
ExecutorServiceinstances with virtual thread supportInfrastructure Changes
selectObjectPageandselectCountByConditionmethods from multiple mapper interfacesLogging Improvements
@Slf4jannotation to enable logging in domain servicesConcurrency Model
CompletableFuture-based asynchronous processing to direct synchronous method calls