Open Codeprh opened 2 years ago
Hi @Codeprh, welcome to SOFAStack community, Please sign Contributor License Agreement!
After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.
Hi @Codeprh, thank for your PR. Please run mvn clean compile -DskipTests
before you commit to format the code.
mvn clean compile -DskipTests
I'm done, I think there is no problem with my code, please help me, thank you~
Hi @Codeprh, thank for your PR. Please run
mvn clean compile -DskipTests
before you commit to format the code.
done,3q
Merging #1213 (ae2e21f) into master (bb7c51b) will increase coverage by
0.52%
. The diff coverage is56.25%
.
@@ Coverage Diff @@
## master #1213 +/- ##
============================================
+ Coverage 71.56% 72.09% +0.52%
+ Complexity 832 780 -52
============================================
Files 408 412 +4
Lines 17224 17403 +179
Branches 2684 2702 +18
============================================
+ Hits 12327 12546 +219
+ Misses 3533 3475 -58
- Partials 1364 1382 +18
Impacted Files | Coverage Δ | |
---|---|---|
...mmon/threadpool/MemorySafeLinkedBlockingQueue.java | 50.00% <50.00%> (ø) |
|
...a/rpc/common/threadpool/MemoryLimitCalculator.java | 61.53% <61.53%> (ø) |
|
.../alipay/sofa/rpc/common/utils/ThreadPoolUtils.java | 95.00% <100.00%> (ø) |
|
...ofa/rpc/registry/zk/ZookeeperProviderObserver.java | 72.50% <0.00%> (-2.50%) |
:arrow_down: |
...a/com/alipay/sofa/rpc/event/LookoutSubscriber.java | 88.63% <0.00%> (-2.28%) |
:arrow_down: |
.../alipay/sofa/rpc/metrics/lookout/RpcLookoutId.java | 87.30% <0.00%> (-1.59%) |
:arrow_down: |
...lipay/sofa/rpc/message/AbstractResponseFuture.java | 56.14% <0.00%> (-0.88%) |
:arrow_down: |
.../java/com/alipay/sofa/rpc/common/RpcConstants.java | 100.00% <0.00%> (ø) |
|
...alipay/sofa/rpc/registry/local/DomainRegistry.java | 94.11% <0.00%> (ø) |
|
.../sofa/rpc/registry/local/DomainRegistryHelper.java | 62.50% <0.00%> (ø) |
|
... and 15 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update bb7c51b...ae2e21f. Read the comment docs.
also see : https://github.com/apache/dubbo/pull/9789
@OrezzerO @Codeprh How about this PR: https://github.com/apache/shenyu/pull/3764 ?
@OrezzerO @Codeprh How about this PR: apache/shenyu#3764 ?
This PR has better handling of exceptions. 👍
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
review again
Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.
Overall summary:
This pull request introduces the "feat: add MemorySafeLinkedBlockingQueue" feature, which involves the creation of the MemoryLimitCalculator
class and modifications to the MemorySafeLinkedBlockingQueue
, MemoryLimiter
, ThreadPoolUtils
, and related test files. The primary goal of these changes is to address the OutOfMemory (OOM) problem caused by java.util.concurrent.LinkedBlockingQueue.
Potential issues:
MemoryLimiter
class. Proper input validation should be performed.MemoryLimitCalculator
's scheduled refresh of maximum available memory does not cause performance issues.MemoryLimiter
.MemorySafeLinkedBlockingQueueTest
to confirm adequate testing coverage and reliability.MemoryLimitCalculator
class.Key findings:
MemoryLimitCalculator
class has been created for calculating and periodically refreshing maximum available memory.MemoryLimiter
class is no longer needed and has been removed, but its removal's impact on the codebase should be double-checked.MemorySafeLinkedBlockingQueue
class has been created as an implementation of the queue using the MemoryLimitCalculator
.Recommendations:
MemoryLimiter
class and ensure that its removal does not cause any integration issues.MemoryLimitCalculator
class to confirm that the OOM problem is indeed solved under different scenarios.MemorySafeLinkedBlockingQueue
file.Summary of key changes:
MemoryLimitCalculator
class to calculate and periodically refresh the maximum available memory.MemoryLimiter
class to handle acquiring and releasing objects in memory.MemorySafeLinkedBlockingQueue
class, which is a queue implementation that uses the MemoryLimiter
.MemorySafeLinkedBlockingQueue
(MemorySafeLinkedBlockingQueueTest
).ThreadPoolUtils
class.Key findings and potential problems:
MemoryLimiter
class. Ensure that proper input validation is performed.MemoryLimitCalculator
's scheduled refresh of maximum available memory does not cause performance issues.MemoryLimiter
.MemorySafeLinkedBlockingQueueTest
to confirm adequate testing coverage and reliability.This pull request makes changes to 4 files related to the MemorySafeLinkedBlockingQueue implementation:
Key changes:
MemoryLimitCalculator.java
file has only a line removed which doesn't seem to impact any functionality.MemoryLimiter.java
file had some formatting changes (e.g., adjustments to field alignment), but no changes to the logic.MemorySafeLinkedBlockingQueue.java
file, where the static variable THE_256_MB
is set to 256MB, and the constructor with no arguments initializes the instance variable maxFreeMemory
with the value of THE_256_MB
.MemorySafeLinkedBlockingQueueTest.java
file has new license comment block and additional lines of whitespace but no changes in the unit tests.Potential problems:
MemorySafeLinkedBlockingQueue.java
file.This patch is for the file ThreadPoolUtilsTest.java
and modifies the test cases for the buildQueue
and buildQueue1
methods. The key changes are:
buildQueue
and buildQueue1
test methods.Potential problems:
Summary of key changes in the patch:
Potential problems:
The removal of the MemoryLimiter class can have a substantial impact on the codebase, and it's essential to ensure that there are no dependencies on this class. As it's not clear whether or not other parts of the code still depend on MemoryLimiter, it's essential to check for any possible integration issues.
While the updated JavaDoc for MemoryLimitCalculator mentions that it can completely solve the OOM problem, it's critical to test the code under different scenarios where an OOM problem could occur, ensuring its correctness and effectiveness. This is important because solving the OOM problem is one of the main goals of this pull request.
Based on this patch, I recommend checking for any dependencies on the MemoryLimiter
class and making sure that the supposed OOM problem is indeed solved by the changes made to the MemoryLimitCalculator
class.
Motivation:
Can completely solve the OOM problem caused by {@link java.util.concurrent.LinkedBlockingQueue}
Modification and Result:
CN
参考dubbo的思想(dubbo-pr),引入MemorySafeLinkedBlockingQueue,解决无界队列可能会导致OOM的问题
EN
Referring to the idea of dubbo(dubbo-pr), introduce MemorySafeLinkedBlockingQueue to solve the problem that unbounded queues may cause OOM