【WIP】debug unit test pipeline #379
Conversation
📝 WalkthroughWalkthroughImport cleanups and formatting; added a test-time GC monitor that can trigger heap dumps; introduced Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as JUnit Test
participant Monitor as UnitTestGCMonitor
participant GC as GarbageCollectorMXBean
participant HS as HotSpotDiagnosticMXBean
participant FS as Filesystem
Note over Test,Monitor: Test enables GC monitoring
Test->>Monitor: startMonitoring()
loop periodic polling
Monitor->>GC: query collection count
alt Full GC count increased (> threshold)
Monitor->>HS: request heap dump (dumpPath.hprof)
HS-->>Monitor: dump written / error
Monitor->>FS: ensure dump dir / skip if exists
Monitor-->>Test: log dump path/result
else below threshold
Monitor-->>Test: continue polling
end
end
Test->>Monitor: stopMonitoring()
Monitor-->>GC: stop polling / thread stops
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit-test.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
| timeout-minutes: 15 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest stable version actions/checkout GitHub Action December 2025
💡 Result:
Latest stable version: actions/checkout v6 — use "uses: actions/checkout@v6". (Checked December 30, 2025). [1][2]
Sources:
[1] actions/checkout GitHub repo.
[2] actions/checkout listing on GitHub Marketplace.
🌐 Web query:
latest stable version actions/setup-java GitHub Action December 2025
💡 Result:
The latest stable release is v5.1.0 (released Dec 4, 2025). [1][2]
Sources:
- actions/setup-java releases (v5.1.0). [1]
- GitHub Marketplace listing for setup-java. [2]
🌐 Web query:
latest stable version ScaCap/action-surefire-report GitHub Action December 2025
💡 Result:
Latest stable version: v1.9.1 (release referenced Oct 2025). [1][2]
Sources:
[1] Dependabot PR bump referencing v1.9.1. [2] Workflow/notification referencing the v1.9.1 bump.
🌐 Web query:
latest stable version codecov/codecov-action GitHub Action December 2025
💡 Result:
Latest stable release: v5.5.2 — released December 9, 2025. [1]
Sources
[1] codecov/codecov-action releases (GitHub).
Update GitHub Actions to latest stable versions.
The workflow uses outdated action versions. Update to the latest releases for security patches and feature improvements:
actions/checkout@v2→v6actions/setup-java@v1→v5.1.0ScaCap/action-surefire-report@v1→v1.9.1codecov/codecov-action@v1→v5.5.2
🧰 Tools
🪛 actionlint (1.7.9)
17-17: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/unit-test.yml around line 17: the workflow pins several
actions to outdated major versions; update the action references to the
requested newer releases—change actions/checkout@v2 to actions/checkout@v6,
actions/setup-java@v1 to actions/[email protected],
ScaCap/action-surefire-report@v1 to ScaCap/[email protected], and
codecov/codecov-action@v1 to codecov/[email protected]; ensure any
inputs/arguments still match the new action versions and run the workflow to
verify no breaking changes.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
42-51: Document why tests are disabled and track re-enablement.This test class is disabled with
@Ignore, reducing test coverage. Add a comment explaining why and create a tracking issue to fix and re-enable these tests.
♻️ Duplicate comments (1)
.github/workflows/unit-test.yml (1)
24-25: JVM heap allocation exceeds runner capacity and will cause pipeline failure.The MAVEN_OPTS configuration allocates 12GB for both maximum and initial heap size. Previous reviews flagged 8GB as too large, but this has been increased to 12GB. GitHub Actions ubuntu-22.04 runners provide only ~7GB of total RAM, so this will cause JVM startup failure or out-of-memory errors, directly contradicting the PR objective of verifying the pipeline works correctly.
🔎 Recommended fix: Reduce heap allocation to fit within runner limits
- name: Unit Test env: - MAVEN_OPTS: "-Xmx12g -Xms12g" + MAVEN_OPTS: "-Xmx6g -Xms2g" run: mvn compile -B && mvn clean test -DisSkipIntegrationTest=true "-Dtest.logging.level=ERROR" --fail-at-end --batch-modeThis allocates 6GB max heap (leaving ~1GB for OS/other processes) and 2GB initial heap (reducing startup memory pressure).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/unit-test.ymlserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushProcessorTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (1)
Ignore(35-331)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
Ignore(51-221)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushProcessorTest.java (1)
Ignore(51-545)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/unit-test.yml (1)
34-43: Test reporting steps added for better visibility.The addition of test report publishing and code coverage steps improves pipeline observability. Ensure the
CODECOV_TOKENsecret is configured in the repository settings for codecov uploads to the protected branch.
.github/workflows/unit-test.yml
Outdated
| runs-on: ubuntu-22.04 | ||
| timeout-minutes: 15 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/checkout@v5 | ||
| - name: Set up JDK 1.8 | ||
| uses: actions/setup-java@v1 | ||
| uses: actions/setup-java@v4 | ||
| with: | ||
| java-version: 1.8 | ||
| - name: Unit Testt | ||
| java-version: '8.0.472+8' | ||
| distribution: 'temurin' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/unit-test.ymlRepository: sofastack/sofa-registry
Length of output: 1241
🌐 Web query:
actions/checkout latest version stable release 2025
💡 Result:
The current stable release of actions/checkout is v6. [1][2]
Sources:
- actions/checkout repository README (shows Checkout v6). [1]
- GitHub Marketplace / actions/checkout listing (Checkout v6 notes). [2]
🌐 Web query:
actions/setup-java latest version stable release 2025
💡 Result:
The latest stable release of actions/setup-java is v5.1.0 (released December 4, 2025). [1][2]
Sources:
- actions/setup-java releases (v5.1.0). [1]
- GitHub Marketplace setup-java (release notes / v5 info). [2]
🌐 Web query:
ScaCap/action-surefire-report latest version 2025
💡 Result:
The latest released version of ScaCap/action-surefire-report is v1.9.1 (released in 2025). [1][2]
Sources:
[1] Dependabot PR / release notes referencing v1.9.1.
[2] Security/usage trackers showing scacap/action-surefire-report v1.9.1 as the most recent release.
🌐 Web query:
codecov/codecov-action latest version stable 2025
💡 Result:
The latest stable release is v5.5.2 (released December 9, 2025). [1][2]
Sources:
- codecov/codecov-action releases (v5.5.2). [1]
- codecov/codecov-action README / repo (v5 recommended; v5 series). [2]
Update GitHub Actions to latest stable versions available as of December 2025.
Several actions in the workflow are outdated:
actions/checkout@v5→ upgrade to v6 (current stable)actions/setup-java@v4→ upgrade to v5.1.0 (released December 4, 2025 with improvements)codecov/codecov-action@v1→ upgrade to v5.5.2 (released December 9, 2025; significantly outdated at 4 major versions behind)ScaCap/action-surefire-report@v1→ consider v1.9.1 for latest patches
Newer versions include security patches, bug fixes, and improved functionality. The codecov action in particular should be upgraded as v1 is severely behind current releases.
🤖 Prompt for AI Agents
In .github/workflows/unit-test.yml around lines 14 to 22, the workflow uses
outdated GitHub Actions versions; update actions to current stable releases:
change actions/checkout@v5 to actions/checkout@v6, actions/setup-java@v4 to
actions/[email protected] (adjust any renamed inputs if present), replace any
usage of codecov/codecov-action@v1 with codecov/[email protected] and update
its invocation syntax if required by the new major version, and bump
ScaCap/action-surefire-report@v1 to v1.9.1; after changes, run the workflow
locally or in a branch to confirm there are no breaking input/parameter name
changes and fix any updated input keys per each action's latest docs.
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.mockito.Mockito; | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
Document why tests are disabled and track re-enablement.
This test class is disabled with @Ignore, reducing test coverage. Add a comment explaining why and create a tracking issue to fix and re-enable these tests.
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushProcessorTest.java
around lines 46-51 the test class is annotated with @Ignore with no rationale;
add a concise comment above the class explaining exactly why the tests are
disabled (e.g., flaky due to X, depends on external service Y, or failing after
change Z), include a reference to a newly created tracking issue (issue number
or URL) to fix and re-enable the tests, add a TODO with owner and target
milestone/ETA, and keep the @Ignore until the tracking issue is resolved so CI
and future reviewers can trace and prioritize re-enablement.
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
| import org.mockito.Mockito; | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
Document why tests are disabled and track re-enablement.
The entire test class is disabled with @Ignore, which reduces test coverage and contradicts the PR objective of verifying the unit test pipeline. If these tests are unstable or blocking pipeline validation, add a comment explaining why they're disabled and create a tracking issue to fix and re-enable them.
🔎 Suggested documentation pattern
+// TODO(issue-XXX): Re-enable after fixing flaky test behavior
@Ignore
public class SessionDataStoreTest {📝 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.
| import org.junit.Ignore; | |
| import org.junit.Test; | |
| import org.mockito.Mockito; | |
| @Ignore | |
| import org.junit.Ignore; | |
| import org.junit.Test; | |
| import org.mockito.Mockito; | |
| // TODO(issue-XXX): Re-enable after fixing flaky test behavior | |
| @Ignore |
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java
around lines 33 to 37, the class is annotated with @Ignore with no explanation;
add a short comment above the annotation describing why the tests are disabled,
the conditions for re-enablement, and reference a created tracking issue or
ticket number, or if tests are stable, remove the @Ignore so they run; ensure
the tracking issue exists in the repo and include the issue ID in the comment
(e.g. "Disabled due to flakiness under CI — see ISSUE-123") so future
maintainers know how and when to re-enable.
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
| import org.springframework.util.CollectionUtils; | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
Document why tests are disabled and track re-enablement.
This test class is disabled with @Ignore, reducing test coverage. Add a comment explaining why and create a tracking issue to fix and re-enable these tests.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.javaclient/impl/src/test/java/com/alipay/sofa/registry/client/provider/RegisterOrderTest.javaserver/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/elector/MetaJdbcLeaderElectorTest.javaserver/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/repository/impl/AppRevisionRepositoryTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/repository/impl/AppRevisionRepositoryTest.java (2)
server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/elector/MetaJdbcLeaderElectorTest.java (1)
Ignore(44-227)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (1)
Ignore(35-331)
client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java (1)
client/impl/src/test/java/com/alipay/sofa/registry/client/provider/RegisterOrderTest.java (1)
Ignore(49-220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/repository/impl/AppRevisionRepositoryTest.java (1)
39-39: Ensure @ignore is removed or documented before merging.This annotation disables all tests in the class, which is appropriate for debugging the pipeline as stated in the PR objectives. However, before merging to master:
- Either remove
@Ignoreafter identifying and fixing the root cause, OR- Add a comment explaining why the tests are disabled (e.g., linking to a tracking issue)
Have the underlying test failures been identified? If these tests are consistently failing, consider investigating the root cause rather than leaving them disabled permanently.
Also applies to: 48-48
| * @author zhuoyu.sjw | ||
| * @version $Id : DefaultRegistryClientTest.java, v 0.1 2017-11-24 20:05 zhuoyu.sjw Exp $$ | ||
| */ | ||
| @Ignore |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the test file exists and examine it
cat -n "client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java" | head -60Repository: sofastack/sofa-registry
Length of output: 3128
🏁 Script executed:
# Count the total test methods in the file
rg "@Test" "client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java" | wc -lRepository: sofastack/sofa-registry
Length of output: 68
🏁 Script executed:
# Check for other @Ignore annotations in test files across the repository
rg "@Ignore" --type java ".*Test\.java$" -c | head -20Repository: sofastack/sofa-registry
Length of output: 120
🏁 Script executed:
# Look at the PR context - check git log or any related comments
git log --oneline -n 20 -- "client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java"Repository: sofastack/sofa-registry
Length of output: 108
🏁 Script executed:
# Check for @Ignore annotations in test files with correct syntax
rg "@Ignore" --type java | grep -i "test\.java" | head -20Repository: sofastack/sofa-registry
Length of output: 977
🏁 Script executed:
# Get the full file to see all test methods
wc -l "client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java"Repository: sofastack/sofa-registry
Length of output: 168
🏁 Script executed:
# Check git history for when @Ignore was added to this specific file
git log -p --follow -- "client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java" | grep -A 10 -B 5 "@Ignore" | head -30Repository: sofastack/sofa-registry
Length of output: 49
🏁 Script executed:
# Count class-level @Ignore annotations in test files
rg "^\s*@Ignore\s*$" --type java -B 1 | grep "\.java-" | wc -lRepository: sofastack/sofa-registry
Length of output: 68
Remove @ignore annotation or create a tracking issue to re-enable these tests before merge.
Disabling the entire test class with @ignore removes 9 test methods covering core client functionality from the test suite. While I understand this appears to be a temporary debugging measure for pipeline verification, there are 8 test classes across the repository currently ignored—this pattern is concerning.
To proceed responsibly:
- Fix the underlying cause of test failures rather than suppressing them
- Remove the @ignore annotation before merging, or document a specific plan and timeline for re-enabling these tests
- If tests must remain disabled temporarily, add an explanatory comment and create a tracking issue
🤖 Prompt for AI Agents
In
client/impl/src/test/java/com/alipay/sofa/registry/client/provider/DefaultRegistryClientTest.java
around line 52, the entire test class is disabled with @Ignore which removes 9
core tests from the suite; either remove the @Ignore to re-enable tests and fix
any failing test causes, or if you truly must keep them disabled temporarily add
a clear comment above the annotation describing why, what will be done, and a
tracking issue ID (create an issue in the repo and reference it), and ensure the
PR includes that issue link and a timeline for re-enablement before merge.
| * @author hui.shih | ||
| * @version $Id: RegisterOrderTest.java, v 0.1 2018-03-07 16:28 hui.shih Exp $$ | ||
| */ | ||
| @Ignore |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the annotation context
head -60 client/impl/src/test/java/com/alipay/sofa/registry/client/provider/RegisterOrderTest.java | tail -20Repository: sofastack/sofa-registry
Length of output: 602
🏁 Script executed:
# Count total test methods in the class (methods starting with @Test or test prefix)
rg -n '(@Test|public void test)' client/impl/src/test/java/com/alipay/sofa/registry/client/provider/RegisterOrderTest.javaRepository: sofastack/sofa-registry
Length of output: 136
🏁 Script executed:
# Check for @Ignore annotations across the entire test module
rg -n '@Ignore' client/impl/src/test/ -C1 | head -50Repository: sofastack/sofa-registry
Length of output: 783
Temporary test disablement should not be merged.
This @Ignore annotation disables all 6 test methods in this class, significantly reducing test coverage for registration order scenarios. While this is appropriate for WIP pipeline debugging, ensure that:
- The root cause of any test failures is investigated and resolved
- The
@Ignoreannotation is removed before merging - All tests pass successfully in the pipeline
Consider creating a tracking issue to ensure these tests are re-enabled.
🤖 Prompt for AI Agents
In
client/impl/src/test/java/com/alipay/sofa/registry/client/provider/RegisterOrderTest.java
around line 49 the class-level @Ignore is disabling all six tests; remove the
@Ignore annotation from the class, run the full test suite locally and in CI to
capture failing tests, diagnose and fix the underlying failures (e.g., adjust
test setup/mocks, timing, or production code as appropriate), re-run until all
six tests pass, and if you cannot fix immediately create a tracking issue
referencing the failing tests and CI logs before any temporary disablement is
reintroduced.
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * @author zhuchen | ||
| * @date Mar 15, 2021, 7:46:21 PM | ||
| */ | ||
| @Ignore |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read its content
if [ -f "server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/elector/MetaJdbcLeaderElectorTest.java" ]; then
echo "=== File exists. Reading content ==="
cat -n "server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/elector/MetaJdbcLeaderElectorTest.java"
else
echo "File not found at specified path. Searching for it..."
find . -name "MetaJdbcLeaderElectorTest.java" -type f
fiRepository: sofastack/sofa-registry
Length of output: 9993
Do not merge test disablement to master.
The @Ignore annotation disables all 5 test methods in this class, including critical leader election tests for distributed locking. While this may be acceptable for temporary pipeline debugging (as indicated by the WIP status), disabling tests without addressing the root cause introduces significant risk:
- Leader election bugs can cause split-brain scenarios or data inconsistency
- Disabled tests mask potential regressions in core functionality
- The actual test failures remain undiagnosed
Before merging to master, investigate why these tests are failing and either fix the tests or the underlying code. If the tests are flaky due to timing issues or environmental dependencies, consider refactoring them rather than disabling them.
🤖 Prompt for AI Agents
In
server/store/jdbc/src/test/java/com/alipay/sofa/registry/jdbc/elector/MetaJdbcLeaderElectorTest.java
around lines 37-44 the class-level @Ignore disables all five leader-election
tests; do not merge this disablement to master. Re-enable the tests (remove the
@Ignore), run the full test suite locally and in CI to capture failures, and fix
the root causes — either correct production code or stabilize tests by replacing
timing-dependent sleeps with deterministic waits/retries, injecting or mocking
external dependencies, and ensuring proper test fixture setup/teardown; if a
temporary CI-only bypass is absolutely required, replace the class-level ignore
with a targeted, documented conditional skip (e.g., System property or @Tag)
referencing a tracked issue so tests remain enabled by default and regressions
are visible.
hui-cha
force-pushed
the
test/test_master_20151230
branch
from
483d5c3 to
48c1261
Compare
hui-cha
force-pushed
the
test/test_master_20151230
branch
from
48c1261 to
faeb7a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit-test.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/unit-test.yml (1)
24-24: Good catch fixing the typo.The step name correction from "Unit Testt" to "Unit Test" improves the workflow's readability.
hui-cha
force-pushed
the
test/test_master_20151230
branch
from
c2cb325 to
ca8434e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/unit-test.yml (1)
19-19: Update GitHub Actions to latest stable versions.Several actions in this workflow are outdated (v2, v1 versions) and should be updated to current stable releases for security patches and improvements. This concern was already raised in previous review comments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/unit-test.ymlpom.xmltest/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/unit-test.yml (1)
26-26: LGTM! Debug flag addition is appropriate for pipeline debugging.Adding the
-Xflag enables Maven debug output, which is suitable for diagnosing unit test pipeline issues as stated in the PR objective. Be aware that this will generate verbose logs.
pom.xml
Outdated
| <!-- <argLine>@{argLine} -Xmx4096m -XX:PermSize=256m -XX:MaxPermSize=512m</argLine>--> | ||
| <argLine>@{argLine} -Xmx12g -XX:PermSize=512m -XX:MaxPermSize=1g</argLine> |
There was a problem hiding this comment.
Critical: JVM heap allocation exceeds GitHub Actions runner capacity and ignores Java 8 flag guidance.
The configuration allocates 12GB heap, but GitHub Actions ubuntu-latest runners provide only ~7GB total RAM. This will cause JVM startup failure, preventing tests from running and contradicting the PR objective.
Additionally, the configuration uses deprecated -XX:PermSize and -XX:MaxPermSize flags despite the comment on Line 635 correctly noting that Java 8 should use -XX:MetaspaceSize and -XX:MaxMetaspaceSize instead.
🔎 Recommended fix: Reduce heap to fit runner limits and follow the Java 8 guidance in the comment
<!-- 根据JDK版本配置，jdk8: -Xmx1024m -XX:MetaspaceSize=256m -XX:MaxMetaspaceSize=512m -->
-<!-- <argLine>@{argLine} -Xmx4096m -XX:PermSize=256m -XX:MaxPermSize=512m</argLine>-->
- <argLine>@{argLine} -Xmx12g -XX:PermSize=512m -XX:MaxPermSize=1g</argLine>
+ <argLine>@{argLine} -Xmx6g -XX:MetaspaceSize=256m -XX:MaxMetaspaceSize=512m</argLine>This allocates 6GB max heap (leaving ~1GB for OS and other processes) and uses the correct Metaspace flags for Java 8 as recommended in the comment above.
📝 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.
| <!-- <argLine>@{argLine} -Xmx4096m -XX:PermSize=256m -XX:MaxPermSize=512m</argLine>--> | |
| <argLine>@{argLine} -Xmx12g -XX:PermSize=512m -XX:MaxPermSize=1g</argLine> | |
| <argLine>@{argLine} -Xmx6g -XX:MetaspaceSize=256m -XX:MaxMetaspaceSize=512m</argLine> |
🤖 Prompt for AI Agents
In pom.xml around lines 636-637, the argLine allocates 12GB and uses deprecated
PermSize flags which will cause JVM startup failures on GitHub Actions runners
and ignores the Java 8 guidance; change the heap to a value that fits the runner
(e.g., -Xmx6g) and replace -XX:PermSize/-XX:MaxPermSize with Java 8 Metaspace
flags (e.g., -XX:MetaspaceSize=512m -XX:MaxMetaspaceSize=1g), keeping the
existing @{argLine} prefix so the final argLine looks like @{argLine} -Xmx6g
-XX:MetaspaceSize=512m -XX:MaxMetaspaceSize=1g.
test/pom.xml
Outdated
| <testNGArtifactName>none:none</testNGArtifactName> | ||
| <junitArtifactName>org.junit:com.springsource.org.junit</junitArtifactName> | ||
| <argLine>@{argLine} -Xmx6144m -XX:PermSize=256m -XX:MaxPermSize=512m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp -Dfile.encoding=UTF8</argLine> | ||
| <argLine>@{argLine} -Xmx12g -XX:PermSize=512m -XX:MaxPermSize=1g -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp -Dfile.encoding=UTF8</argLine> |
There was a problem hiding this comment.
Critical: JVM heap allocation exceeds GitHub Actions runner capacity and uses deprecated Java 7 flags.
The configuration allocates 12GB heap, but GitHub Actions ubuntu-latest runners provide only ~7GB total RAM. This will prevent the JVM from starting, causing immediate pipeline failure—contradicting the PR objective of verifying the unit test pipeline works correctly.
Additionally, -XX:PermSize and -XX:MaxPermSize are Java 7 flags that were removed in Java 8+ (replaced by Metaspace). Since the workflow uses JDK 1.8, these flags are deprecated and ignored.
🔎 Recommended fix: Reduce heap to fit runner limits and use correct Java 8 flags
- <argLine>@{argLine} -Xmx12g -XX:PermSize=512m -XX:MaxPermSize=1g -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp -Dfile.encoding=UTF8</argLine>
+ <argLine>@{argLine} -Xmx6g -XX:MetaspaceSize=256m -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp -Dfile.encoding=UTF8</argLine>This allocates 6GB max heap (leaving ~1GB for OS and other processes) and uses the correct Metaspace flags for Java 8+.
📝 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.
| <argLine>@{argLine} -Xmx12g -XX:PermSize=512m -XX:MaxPermSize=1g -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp -Dfile.encoding=UTF8</argLine> | |
| <argLine>@{argLine} -Xmx6g -XX:MetaspaceSize=256m -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/tmp -Dfile.encoding=UTF8</argLine> |
🤖 Prompt for AI Agents
In test/pom.xml around line 125, the current argLine sets -Xmx12g and Java 7
PermSize flags which exceed GitHub Actions runner RAM and use deprecated JVM
options; change the JVM options to a max heap that fits the runner (e.g. -Xmx6g)
remove -XX:PermSize and -XX:MaxPermSize and replace with Java 8+ metaspace flags
(e.g. -XX:MaxMetaspaceSize=256m), while keeping -XX:+HeapDumpOnOutOfMemoryError,
-XX:HeapDumpPath=/tmp and -Dfile.encoding=UTF8 so the JVM can start reliably on
CI.
ca8434e to
c41a65f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.java (1)
29-33: Test exits immediately without waiting or asserting.The test starts monitoring but returns immediately, so the monitor thread will be killed when the JVM exits. For meaningful GC observation, either:
- Add a delay/wait for the monitor to run through at least one cycle
- Add assertions to verify expected behavior
Given the WIP/debug nature of this PR, this may be intentional for initial verification, but the test won't validate actual GC monitoring functionality in its current form.
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UnitTestGCMonitor.java (2)
110-111: Inappropriate use of ERROR log level for operational messages.Throughout this class,
LOGGER.error()is used for normal operational/debug messages. This will pollute error logs and make genuine errors harder to identify. Consider using:
LOGGER.info()for operational status (monitoring started/stopped)LOGGER.debug()for per-cycle GC bean detailsLOGGER.warn()for skip conditionsLOGGER.error()only for actual failures
137-137: Use platform-independent path construction.Hardcoded
/separator may cause issues on Windows. UsePaths.get()for cross-platform compatibility.🔎 Proposed fix
- String dumpPath = this.dumpDir + "/" + gcBeanName + ".hprof"; + String dumpPath = Paths.get(this.dumpDir, gcBeanName + ".hprof").toString();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.javaserver/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UnitTestGCMonitor.java
💤 Files with no reviewable changes (1)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java
✅ Files skipped from review due to trivial changes (2)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UnitTestGCMonitor.java (1)
19-19: Internal JDK API may not be available on all JVMs.
com.sun.management.HotSpotDiagnosticMXBeanis a HotSpot-specific internal API. This will fail on non-HotSpot JVMs (e.g., OpenJ9, GraalVM native-image). Since this is test infrastructure, it's likely acceptable, but consider adding a check or fallback.
| private Lock lock; | ||
|
|
||
| private boolean running; | ||
|
|
||
| private Thread monitoryThread; |
There was a problem hiding this comment.
Critical bug: running flag is never set to true.
The running field is initialized to false but never updated when monitoring starts. This causes:
stopMonitoring()will always return early (line 88-91) sincerunningis alwaysfalse- Duplicate calls to
startMonitoring()won't be detected
Also, lock should be final, and there's a typo: monitoryThread → monitorThread.
🔎 Proposed fix
- private Lock lock;
+ private final Lock lock;
- private boolean running;
+ private volatile boolean running;
- private Thread monitoryThread;
+ private Thread monitorThread;And in startMonitoring():
LOGGER.error("[UnitTestGCMonitor] 开始监控 GC 情况");
+ this.running = true;
Thread monitoryThread = new Thread(this, "UnitTestGCMonitorThread");And in stopMonitoring():
// 中断线程，然后退出
- this.monitoryThread.interrupt();
- this.monitoryThread = null;
+ this.monitorThread.interrupt();
+ this.monitorThread = null;
+ this.running = false;📝 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.
| private Lock lock; | |
| private boolean running; | |
| private Thread monitoryThread; | |
| private final Lock lock; | |
| private volatile boolean running; | |
| private Thread monitorThread; |
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UnitTestGCMonitor.java
around lines 48-52, the running flag is declared but never set to true, lock
should be final, and monitoryThread is misspelled; update declarations to: make
Lock lock final, rename monitoryThread to monitorThread, and ensure
startMonitoring() sets running = true when monitoring actually starts (and
returns early if already running), while stopMonitoring() should set running =
false after stopping and rely on the running flag to avoid early returns; also
update any usages/tests to reference the renamed monitorThread.
| if (Thread.interrupted()) { | ||
| // 响应中断 | ||
| LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); | ||
| } |
There was a problem hiding this comment.
Interrupt handling doesn't exit the loop.
After detecting an interrupt, the code logs but continues the while(true) loop instead of breaking out.
🔎 Proposed fix
if (Thread.interrupted()) {
// 响应中断
LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!");
+ return;
}📝 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.
| if (Thread.interrupted()) { | |
| // 响应中断 | |
| LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); | |
| } | |
| if (Thread.interrupted()) { | |
| // 响应中断 | |
| LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UnitTestGCMonitor.java
around lines 166 to 169, the code logs when Thread.interrupted() is true but
continues the while(true) loop; change the interrupt handling to restore the
interrupt status and exit the loop (e.g., call
Thread.currentThread().interrupt() and then break or return) so the thread
actually stops after logging.
| } catch (Throwable throwable) { | ||
| LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出" + throwable); | ||
| } |
There was a problem hiding this comment.
Exception logging loses stack trace.
String concatenation with Throwable only captures toString(), losing the full stack trace. Use the logging framework's throwable parameter.
🔎 Proposed fix
} catch (Throwable throwable) {
- LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出" + throwable);
+ LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出", throwable);
}📝 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.
| } catch (Throwable throwable) { | |
| LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出" + throwable); | |
| } | |
| } catch (Throwable throwable) { | |
| LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出", throwable); | |
| } |
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UnitTestGCMonitor.java
around lines 171 to 173, the catch block logs the Throwable via string
concatenation which only calls toString() and loses the stack trace; update the
LOGGER.error call to pass a descriptive message as the first argument and the
caught throwable as the second parameter (i.e., use the logger's throwable
overload) so the full stack trace is preserved in the logs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.java (1)
30-34: Consider adding a test timeout and assertions.The test lacks assertions and a timeout, which means:
- It will always pass unless an exception is thrown
- It could potentially run indefinitely if
startMonitoring()blocksFor a more robust test:
- Add
@Test(timeout = ...)to prevent hanging in CI/CD- Include assertions to verify the monitoring behavior
🔎 Example with timeout and basic verification
- @Test + @Test(timeout = 15000) // 15 second timeout public void test() { + String tempDir = System.getProperty("java.io.tmpdir"); - UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); + UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor(tempDir, 100, 10, TimeUnit.SECONDS); unitTestGCMonitor.startMonitoring(); + // Add assertion or verification here + assertTrue("Monitor should be running", unitTestGCMonitor.isMonitoring()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/AllTests.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/AllTests.java (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.java (1)
UTHeapWatcherTest(27-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/AllTests.java (1)
26-26: No action required.The
UnitTestGCMonitorclass already exists in the codebase and is properly implemented. TheUTHeapWatcherTestclass successfully instantiates it, confirming full compatibility. No verification is needed.Likely an incorrect or invalid review comment.
| @Test | ||
| public void test() { | ||
| UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); | ||
| unitTestGCMonitor.startMonitoring(); | ||
| } |
There was a problem hiding this comment.
Add cleanup to stop monitoring after the test completes.
The test starts monitoring but never stops it, which can leave background threads or resources active after the test completes. This may interfere with subsequent tests or cause resource leaks in the CI/CD pipeline.
🔎 Proposed fix to add cleanup
@Test
public void test() {
UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS);
unitTestGCMonitor.startMonitoring();
+ try {
+ // Allow monitoring to run for the test duration
+ TimeUnit.SECONDS.sleep(1);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ } finally {
+ unitTestGCMonitor.stopMonitoring(); // Ensure cleanup
+ }
}Note: Verify that UnitTestGCMonitor has a stopMonitoring() or similar cleanup method.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.java
around lines 30 to 34, the test starts UnitTestGCMonitor but never stops it;
update the test to ensure monitoring is stopped after completion by calling the
monitor's cleanup method (e.g., stopMonitoring()) — wrap the start and test
logic in a try/finally (or use an @After teardown) and invoke
unitTestGCMonitor.stopMonitoring() in the finally/teardown to guarantee
background threads/resources are released.
| /** 这个方法用于监控堆内存使用情况 异常的话，尝试 Dump */ | ||
| @Test | ||
| public void test() { | ||
| UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Replace hard-coded /tmp path with a cross-platform alternative.
The hard-coded /tmp path won't work on Windows systems, which could cause the unit test pipeline to fail on Windows build agents.
🔎 Proposed fix using system temp directory
+ @Test
+ public void test() {
+ String tempDir = System.getProperty("java.io.tmpdir");
- UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS);
+ UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor(tempDir, 100, 10, TimeUnit.SECONDS);
unitTestGCMonitor.startMonitoring();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/UTHeapWatcherTest.java
around line 32, the test constructs UnitTestGCMonitor with a hard-coded "/tmp"
directory which fails on Windows; replace the hard-coded path with a
cross-platform system temp directory (e.g., use
System.getProperty("java.io.tmpdir") or create a temporary directory via
Files.createTempDirectory and pass that path) and ensure any created temp
directory is cleaned up after the test.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
core/src/test/java/com/alipay/sofa/registry/core/utils/UTHeapWatcherTest.java (2)
30-34: Missing cleanup: monitoring never stopped.The test starts monitoring but never calls
stopMonitoring(), which can leave background threads or resources active after the test completes, potentially interfering with subsequent tests or causing resource leaks in the CI/CD pipeline.
32-32: Hard-coded/tmppath not cross-platform.The hard-coded
/tmppath will fail on Windows systems. UseSystem.getProperty("java.io.tmpdir")instead.core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java (3)
48-52: Critical bug:runningflag never set totrue, plus field issues.The
runningfield is initialized tofalsebut never updated when monitoring starts. This causes:
stopMonitoring()will always return early (lines 88-91) sincerunningis alwaysfalse- Duplicate calls to
startMonitoring()won't be properly detectedAdditionally:
lockshould befinal- Typo:
monitoryThread→monitorThread
166-169: Interrupt handling doesn't exit the loop.After detecting an interrupt on line 166, the code logs a message but continues the
while(true)loop instead of breaking out or returning, preventing the thread from actually stopping.
171-173: Exception logging loses stack trace.String concatenation with
Throwableon line 172 only capturestoString(), losing the full stack trace. Pass the throwable as a second parameter to the logger.
🧹 Nitpick comments (1)
core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java (1)
185-216: Heap dump generation logic is sound.The implementation correctly:
- Obtains the HotSpotDiagnosticMXBean proxy
- Creates parent directories if needed
- Performs the heap dump with proper exception handling
Note: The null check at line 194 is defensive but redundant, as
newPlatformMXBeanProxythrows an exception rather than returning null on failure.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/test/java/com/alipay/sofa/registry/core/utils/UTHeapWatcherTest.javacore/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/AllTests.java
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java (1)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
LoggerFactory(23-66)
🪛 GitHub Actions: integration test
core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java
[error] 63-63: diamond operator is not supported in -source 1.6 (use -source 7 or higher to enable diamond operator)
🪛 GitHub Actions: unit test
core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java
[error] 63-63: diamond operator is not supported in -source 1.6 (use -source 7 or higher). This causes the test compilation to fail during registry-core's testCompile.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/AllTests.java (1)
27-27: Inconsistency between summary and annotated changes.The AI-generated summary states that
DataCacheTestwas added to the test suite (both the import at Line 27 and the suite class reference at Line 34). However, these lines are not marked as changed in the annotated code—only the blank line at Line 23 appears to have been modified.Please verify whether
DataCacheTestis a new addition in this PR or was already present in the file.Also applies to: 34-34
core/src/test/java/com/alipay/sofa/registry/core/utils/UTHeapWatcherTest.java (1)
1-21: License header and imports look good.Standard Apache 2.0 license and necessary imports are present.
core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java (2)
1-34: License header and imports are correct.All necessary imports for GC monitoring, file I/O, concurrency, and logging are present.
176-183: FGC detection logic looks appropriate.The method correctly identifies common Full GC / Major GC collectors for JDK 8 (Parallel, Serial, G1, CMS).
| this.monitorIntervalMs = timeUnit.toMillis(monitorInterval); | ||
| this.lock = new ReentrantLock(false); | ||
| this.running = false; | ||
| this.countMap = new HashMap<>(); |
There was a problem hiding this comment.
CRITICAL: Diamond operator causes pipeline failure.
The diamond operator <> on line 63 requires Java 7+ but the project is compiled with -source 1.6, causing the unit test pipeline to fail during test compilation.
🔎 Proposed fix to remove diamond operator
- this.countMap = new HashMap<>();
+ this.countMap = new HashMap<String, Long>();📝 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.
| this.countMap = new HashMap<>(); | |
| this.countMap = new HashMap<String, Long>(); |
🧰 Tools
🪛 GitHub Actions: integration test
[error] 63-63: diamond operator is not supported in -source 1.6 (use -source 7 or higher to enable diamond operator)
🪛 GitHub Actions: unit test
[error] 63-63: diamond operator is not supported in -source 1.6 (use -source 7 or higher). This causes the test compilation to fail during registry-core's testCompile.
🤖 Prompt for AI Agents
In core/src/test/java/com/alipay/sofa/registry/core/utils/UnitTestGCMonitor.java
around line 63, the assignment uses the diamond operator (this.countMap = new
HashMap<>();) which is incompatible with the project’s -source 1.6; replace the
diamond with explicit generic type parameters matching the countMap declaration
(e.g., new HashMap<String, AtomicInteger>() or whatever types countMap was
declared with), and ensure any required imports are present so the code compiles
under Java 1.6.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
76-79: Variable shadowing in thread initialization.The local variable
monitoryThreadon line 76 shadows the fieldthis.monitoryThread. This issue is part of the broadermonitoryThreadnaming and lifecycle management problem already flagged in the past review comments for lines 48-52.Based on the previous review, after renaming
monitoryThreadtomonitorThreadand making it final, this section should directly assign to the field without a local variable.
🧹 Nitpick comments (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (1)
36-38: Simplify initialization to avoid variable shadowing.The local variable
unitTestGCMonitoron line 36 shadows the field name and adds unnecessary indirection.🔎 Proposed refactor
- UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); - unitTestGCMonitor.startMonitoring(); - this.unitTestGCMonitor = unitTestGCMonitor; + this.unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); + this.unitTestGCMonitor.startMonitoring();server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
32-37: Use the custom LoggerFactory for consistency.The codebase provides a custom
com.alipay.sofa.registry.log.LoggerFactorywrapper (as shown in the relevant code snippets), but this code directly usesorg.slf4j.LoggerFactory.🔎 Proposed refactor for consistency
-import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import com.alipay.sofa.registry.log.Logger; +import com.alipay.sofa.registry.log.LoggerFactory;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java
🧰 Additional context used
🧬 Code graph analysis (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
UnitTestGCMonitor(35-217)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
LoggerFactory(23-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
153-156: Verify the early return behavior after successful heap dump.After a successful heap dump, the method returns on line 156, which exits the monitoring loop entirely. This means:
- Monitoring stops after the first successful dump
- No further GC events will be monitored
- The thread terminates
If the intention is to continuously monitor and potentially create multiple dumps (with different GC bean names), the
returnshould be removed or changed tobreakthe inner loop.Is this one-shot behavior intentional for debugging purposes, or should monitoring continue after a dump?
| private UnitTestGCMonitor unitTestGCMonitor; | ||
|
|
||
| public PushEfficiencyConfigUpdaterTest() { | ||
| UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); | ||
| unitTestGCMonitor.startMonitoring(); | ||
| this.unitTestGCMonitor = unitTestGCMonitor; | ||
| } |
There was a problem hiding this comment.
Critical resource leak: GC monitor thread never stopped.
The GC monitor is started in the constructor but there's no corresponding cleanup in an @After method. Since JUnit creates a new test instance for each test method, this will:
- Start a new monitoring thread for each test method
- Leave threads running after tests complete
- Potentially cause pipeline hangs or resource exhaustion
🔎 Proposed fix: Add cleanup method
+ @After
+ public void tearDown() {
+ if (this.unitTestGCMonitor != null) {
+ this.unitTestGCMonitor.stopMonitoring();
+ }
+ }Also add the import:
import org.junit.After;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java
around lines 33 to 39, the UnitTestGCMonitor is started in the constructor but
never stopped; add a JUnit @After annotated method (and import org.junit.After)
that checks if unitTestGCMonitor is non-null and calls its stopMonitoring() (or
the monitor's proper shutdown/stop method) to terminate the background thread
and prevent resource leaks.
| private UnitTestGCMonitor unitTestGCMonitor; | ||
|
|
||
| public PushEfficiencyConfigUpdaterTest() { | ||
| UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Hardcoded /tmp path is not portable to Windows.
Using a hardcoded Unix path may cause issues if tests run on Windows environments.
🔎 Proposed fix for cross-platform compatibility
- UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS);
+ String tmpDir = System.getProperty("java.io.tmpdir");
+ UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor(tmpDir, 100, 10, TimeUnit.SECONDS);📝 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.
| UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor("/tmp", 100, 10, TimeUnit.SECONDS); | |
| String tmpDir = System.getProperty("java.io.tmpdir"); | |
| UnitTestGCMonitor unitTestGCMonitor = new UnitTestGCMonitor(tmpDir, 100, 10, TimeUnit.SECONDS); |
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java
around line 36, the test constructs UnitTestGCMonitor with a hardcoded "/tmp"
path which is not portable to Windows; replace the hardcoded path with a
platform-independent temporary directory (e.g., use
System.getProperty("java.io.tmpdir") or create an ephemeral temp directory via
Files.createTempDirectory and clean it up after the test, or use JUnit's
TemporaryFolder/TempDir) and pass that path to UnitTestGCMonitor so the test
runs correctly on all OSes.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In
@server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java:
- Around line 49-52: The test currently starts GC monitoring in the
BoltEncodeTest constructor causing repeated starts and no stop; replace the
constructor logic by adding a static @BeforeClass method that calls
UnitTestGCMonitor.getInstance().startMonitoring() and a static @AfterClass
method that calls UnitTestGCMonitor.getInstance().stopMonitoring(); ensure both
lifecycle methods are static so monitoring runs once for the test class and is
properly cleaned up, and remove the startMonitoring() call from the
BoltEncodeTest() constructor.
♻️ Duplicate comments (3)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (3)
51-55: Critical bugs already flagged in past review.Multiple critical issues were identified in previous reviews:
- Line 80: Diamond operator
<>is not supported in Java 1.6, causing compilation failure- Line 51:
lockshould befinal- Line 53:
runningflag is never set totrue, breaking start/stop logic- Line 55: Typo
monitoryThreadshould bemonitorThreadPlease address the fixes proposed in the past review comments.
Also applies to: 80-80
121-121: Run loop doesn't check therunningflag.The monitoring loop uses
while(true)and never checks therunningflag, making graceful shutdown viastopMonitoring()impossible without interruption. Even after therunningflag bug is fixed, the loop should periodically checkrunningto allow clean shutdown.Additionally, as flagged in past reviews, the interrupt handling on lines 183-186 doesn't exit the loop.
🔎 Proposed fix
- while (true) { + while (this.running) { // ... monitoring logic ... LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(monitorIntervalMs)); if (Thread.interrupted()) { // 响应中断 LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); + return; } }Also applies to: 181-186
188-190: Exception logging loses stack trace (duplicate).As flagged in past reviews, line 189 concatenates the throwable with a string, losing the stack trace. This issue was already identified.
🧹 Nitpick comments (3)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (3)
20-21: Use the project's LoggerFactory wrapper.The code imports
org.slf4j.Loggerandorg.slf4j.LoggerFactorydirectly, but the codebase provides a wrapper atcom.alipay.sofa.registry.log.LoggerFactory(as shown in the relevant snippets). Use the project's wrapper for consistency.🔎 Proposed fix
-import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import com.alipay.sofa.registry.log.Logger; +import com.alipay.sofa.registry.log.LoggerFactory;
91-91: Misuse of error-level logging for non-error messages.Throughout the class,
LOGGER.error()is used to log informational and debug-level messages (e.g., "开始监控 GC 情况", "获取到 GC Bean", etc.). This pollutes error logs and makes it difficult to identify actual errors. Use appropriate log levels:info()for informational messages,debug()for detailed diagnostic output, and reserveerror()for actual error conditions.Example fixes
- LOGGER.error("[UnitTestGCMonitor] 开始监控 GC 情况"); + LOGGER.info("[UnitTestGCMonitor] 开始监控 GC 情况"); - LOGGER.error( - "[UnitTestGCMonitor] 获取到 GC Bean: {}, 当前 GC 次数: {}", gcBeanName, currentGCCount); + LOGGER.debug( + "[UnitTestGCMonitor] 获取到 GC Bean: {}, 当前 GC 次数: {}", gcBeanName, currentGCCount);Also applies to: 127-128, 134-134, 142-146, 157-160, 164-169, 171-172, 175-176, 185-185
93-96: Local variable shadows instance field.Line 93 declares a local variable
monitoryThreadthat shadows the instance field on line 55. While line 96 correctly assigns to the instance field usingthis.monitoryThread, this pattern is confusing. Consider either:
- Directly assigning to
this.monitoryThreadwithout the local variable- Renaming the local variable to avoid shadowing
🔎 Proposed fix
- Thread monitoryThread = new Thread(this, "UnitTestGCMonitorThread"); - monitoryThread.start(); - - this.monitoryThread = monitoryThread; + this.monitoryThread = new Thread(this, "UnitTestGCMonitorThread"); + this.monitoryThread.start();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/ChangeProcessorTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java
✅ Files skipped from review due to trivial changes (1)
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/ChangeProcessorTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
UnitTestGCMonitor(36-234)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
LoggerFactory(23-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java (1)
31-31: Imports look good.The added imports for
UnitTestGCMonitorand JUnit classes are appropriate for the new constructor.Also applies to: 35-37
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
193-233: Helper methods look good with minor caveat.The
isFGC()method correctly identifies Full GC collectors for common JDK 8 GC algorithms. ThegenerateHeapDump()method has proper error handling, directory creation, and correctly logs exceptions with stack traces (line 230).Note: The use of
com.sun.management.HotSpotDiagnosticMXBean(line 19) is a proprietary HotSpot-specific API and won't work on non-HotSpot JVMs, but this is acceptable for a test utility.
| public BoltEncodeTest() { | ||
| UnitTestGCMonitor unitTestGCMonitor = UnitTestGCMonitor.getInstance(); | ||
| unitTestGCMonitor.startMonitoring(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Use JUnit lifecycle methods instead of constructor.
Starting GC monitoring in the test constructor has several issues:
- JUnit creates a new instance for each test method, causing multiple
startMonitoring()calls - No corresponding
stopMonitoring()is visible, risking thread/resource leaks - The singleton pattern means subsequent constructor calls will attempt to start an already-running monitor (though the
runningflag bug inUnitTestGCMonitormasks this)
Move monitoring initialization to a @BeforeClass static method and cleanup to @AfterClass:
🔎 Proposed refactor
- public BoltEncodeTest() {
- UnitTestGCMonitor unitTestGCMonitor = UnitTestGCMonitor.getInstance();
- unitTestGCMonitor.startMonitoring();
- }
+ @BeforeClass
+ public static void setUpClass() {
+ UnitTestGCMonitor.getInstance().startMonitoring();
+ }
+
+ @AfterClass
+ public static void tearDownClass() {
+ UnitTestGCMonitor.getInstance().stopMonitoring();
+ }🤖 Prompt for AI Agents
In
@server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java
around lines 49-52, The test currently starts GC monitoring in the
BoltEncodeTest constructor causing repeated starts and no stop; replace the
constructor logic by adding a static @BeforeClass method that calls
UnitTestGCMonitor.getInstance().startMonitoring() and a static @AfterClass
method that calls UnitTestGCMonitor.getInstance().stopMonitoring(); ensure both
lifecycle methods are static so monitoring runs once for the test class and is
properly cleaned up, and remove the startMonitoring() call from the
BoltEncodeTest() constructor.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java (1)
48-51: Duplicate issue: Use JUnit lifecycle methods instead of constructor.This implementation issue was already flagged in a previous review. The constructor-based approach causes
startMonitoring()to be called for each test method execution without a corresponding cleanup, risking resource leaks and duplicate monitoring threads.server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (6)
50-54: Critical bugs remain unaddressed from previous review.Multiple critical issues flagged in the previous review are still present:
runningflag (line 52) is never set totrueinstartMonitoring(), breaking the state trackinglock(line 50) should befinalmonitoryThread(line 54) contains a typo (should bemonitorThread)
79-79: CRITICAL: Diamond operator breaks Java 1.6 compilation.This issue was flagged in the previous review and remains unaddressed. The diamond operator
<>is incompatible with-source 1.6, causing the unit test pipeline to fail during compilation.🔎 Required fix
- this.countMap = new HashMap<>(); + this.countMap = new HashMap<String, Long>();
82-99: Critical bug:runningflag never set totrue.The
startMonitoring()method checks therunningflag (line 85) but never sets it totrueafter starting the thread. This breaks the idempotency check, allowing multiple monitoring threads to be created.🔎 Required fix
LOGGER.error("[UnitTestGCMonitor] 开始监控 GC 情况"); + this.running = true; Thread monitoryThread = new Thread(this, "UnitTestGCMonitorThread");
101-115: Bug:stopMonitoring()must updaterunningflag.The method interrupts the thread but doesn't set
running = false, leaving the state inconsistent. Also uses the misspelled field namemonitoryThread.🔎 Required fix
// 中断线程，然后退出 - this.monitoryThread.interrupt(); - this.monitoryThread = null; + this.monitorThread.interrupt(); + this.monitorThread = null; + this.running = false;
182-185: Bug: Interrupt handling doesn't exit the loop.After detecting
Thread.interrupted(), the code logs but continues thewhile(true)loop. The thread never actually terminates on interrupt.🔎 Required fix
if (Thread.interrupted()) { // 响应中断 LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); + return; }
187-189: Bug: Exception logging loses stack trace.String concatenation with
Throwableonly capturestoString(), losing diagnostic information.🔎 Required fix
} catch (Throwable throwable) { - LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出" + throwable); + LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出", throwable); }
🧹 Nitpick comments (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (2)
164-164: Consider polling-based waiting instead of fixed sleeps.The fixed 300ms sleeps may be insufficient on slower CI environments or unnecessarily slow on faster machines. Consider refactoring to use polling with timeout (e.g., Awaitility library or custom polling logic) to wait for the expected state, which would make the test both faster and more reliable.
Also applies to: 191-191
226-249: Consider making the inner class static.
MockFetchSystemPropertyAnswerdoesn't reference the outer class and can be declaredstatic. This clarifies that it's independent and reduces memory overhead slightly.🔎 Suggested change
-class MockFetchSystemPropertyAnswer implements Answer<FetchSystemPropertyResult> { +static class MockFetchSystemPropertyAnswer implements Answer<FetchSystemPropertyResult> {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.javaserver/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchPersistenceSystemProperty.javaserver/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchSystemPropertyService.java
🧰 Additional context used
🧬 Code graph analysis (3)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (3)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/URL.java (1)
URL(32-244)server/common/util/src/main/java/com/alipay/sofa/registry/util/JsonUtils.java (1)
JsonUtils(27-71)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1)
ValueConstants(26-200)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java (1)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
UnitTestGCMonitor(35-233)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
LoggerFactory(23-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
76-214: Excellent fix for resource cleanup.Wrapping the test logic in a try-finally block and ensuring
shutdownWatchDog()is called addresses the memory leak concerns mentioned in the PR. The background watchdog thread started at line 161 will now be properly terminated even if the test fails, preventing thread accumulation across test runs.server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchPersistenceSystemProperty.java (1)
130-133: LGTM!The addition of
shutdownWatchDog()provides a proper test-only hook for cleanup. The@VisibleForTestingannotation correctly signals this is not for production use.server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffServiceTest.java (1)
95-95: LGTM!Proper test cleanup. Adding
shutdownWatchDog()in the@AfterClassmethod ensures the watchdog thread is terminated after tests complete, preventing resource leaks.server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchSystemPropertyService.java (1)
169-172: LGTM!The
shutdownWatchDog()method provides proper test cleanup capabilities. The@VisibleForTestingannotation correctly indicates this is for testing purposes only.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In
@server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java:
- Around line 251-254: Move the shutdown/cleanup calls for pubExecutor,
unpubExecutor, clientOffExecutor, and refreshTimer out of the try block and into
a finally block so they always run even if assertions fail; locate the test
method in SessionDataStoreTest where pubExecutor.shutdown(),
unpubExecutor.shutdown(), clientOffExecutor.shutdown(), and
refreshTimer.cancel() are currently invoked and wrap the try { ... } with a
finally { pubExecutor.shutdown(); unpubExecutor.shutdown();
clientOffExecutor.shutdown(); refreshTimer.cancel(); } to guarantee resource
cleanup.
♻️ Duplicate comments (4)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (4)
50-54: Fix field declarations and therunningflag bug.Multiple issues with field declarations:
- Critical: The
runningflag is never set totrueinstartMonitoring(), causingstopMonitoring()to always return early at line 104.- The
lockfield should befinalsince it's initialized once and never reassigned.- Typo:
monitoryThreadshould bemonitorThread.🔎 Proposed fix
- private Lock lock; + private final Lock lock; - private boolean running; + private volatile boolean running; - private Thread monitoryThread; + private Thread monitorThread;Then in
startMonitoring()at line 90-95:LOGGER.error("[UnitTestGCMonitor] 开始监控 GC 情况"); + this.running = true; - Thread monitoryThread = new Thread(this, "UnitTestGCMonitorThread"); - monitoryThread.start(); + Thread monitorThread = new Thread(this, "UnitTestGCMonitorThread"); + monitorThread.start(); - this.monitoryThread = monitoryThread; + this.monitorThread = monitorThread;And in
stopMonitoring()at line 110-111:// 中断线程，然后退出 - this.monitoryThread.interrupt(); - this.monitoryThread = null; + this.monitorThread.interrupt(); + this.monitorThread = null; + this.running = false;
79-79: Critical: Diamond operator causes compilation failure with Java 1.6.The diamond operator
<>requires Java 7+, but this project is compiled with-source 1.6, causing the unit test pipeline to fail during compilation.🔎 Proposed fix
- this.countMap = new HashMap<>(); + this.countMap = new HashMap<String, Long>();
182-185: Interrupt handling doesn't exit the loop.After detecting and logging the thread interrupt, the code continues in the
while(true)loop instead of exiting. This prevents the monitoring thread from stopping properly.🔎 Proposed fix
if (Thread.interrupted()) { // 响应中断 LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); + return; }
187-189: Exception logging loses stack trace.String concatenation with the
Throwableonly capturestoString(), losing the full stack trace. Use the logger's throwable parameter to preserve diagnostic information.🔎 Proposed fix
} catch (Throwable throwable) { - LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出" + throwable); + LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出", throwable); }
🧹 Nitpick comments (3)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
77-220: Excellent resource cleanup pattern to prevent leaks.The try-finally block with null-safe
shutdownWatchDog()calls ensures proper cleanup of background threads even when assertions fail. This directly addresses the memory leak concerns mentioned in the PR objectives.
Optional: Consider more robust test synchronization.
The test currently uses fixed 300ms sleeps (lines 166, 193) to wait for background processing. This could be flaky on slow CI systems. Consider using a polling approach with timeout instead:
// Example polling helper private void waitUntil(Supplier<Boolean> condition, long timeoutMs) { long deadline = System.currentTimeMillis() + timeoutMs; while (!condition.get() && System.currentTimeMillis() < deadline) { sleep(50L, TimeUnit.MILLISECONDS); } } // Usage: waitUntil(() -> fetchDataInfoIDBlackListService.getStorage().get() != null && fetchDataInfoIDBlackListService.getStorage().get().getVersion() == 1L, 1000L);This makes the test more resilient to timing variations while still failing fast when conditions are met.
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java (2)
34-36: Consider using explicit imports instead of wildcards.While acceptable in test code, wildcard imports (
import java.util.*) can reduce clarity and potentially cause conflicts if classes with the same name exist in multiple packages.
144-147: Consider simplifying variable naming.The pattern of creating
storeInstanceand immediately aliasing it asstoreadds minor cognitive overhead. You could simplify by usingstoreInstanceconsistently throughout the test, or by renaming the finally variable to match the test body variable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/AbstractDataManager.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/DataIndexer.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/DataCacheTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java
🧰 Additional context used
🧬 Code graph analysis (4)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/DataCacheTest.java (4)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/DataInfo.java (1)
DataInfo(29-180)server/common/util/src/main/java/com/alipay/sofa/registry/net/NetUtil.java (1)
NetUtil(42-228)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/URL.java (1)
URL(32-244)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1)
ValueConstants(26-200)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.java (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/TestUtils.java (1)
TestUtils(66-502)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/ConnectId.java (1)
ConnectId(31-151)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
LoggerFactory(23-66)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java (4)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/ConnectId.java (1)
ConnectId(31-151)server/common/util/src/main/java/com/alipay/sofa/registry/util/StringFormatter.java (1)
StringFormatter(22-29)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/PublisherUtils.java (1)
PublisherUtils(36-119)server/common/util/src/main/java/com/alipay/sofa/registry/util/ConcurrentUtils.java (1)
ConcurrentUtils(34-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (14)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
38-46: LGTM: Import reorganization is clean.The import reordering (static imports after non-static) and addition of necessary imports are straightforward formatting changes with no functional impact.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/AbstractDataManager.java (2)
25-25: LGTM - Import organization improved.The addition of
@VisibleForTestingand the replacement of wildcard imports with explicit imports improve code clarity.Also applies to: 32-34
259-262: LGTM - Testing hook enables proper resource cleanup.The
shutdownWatchDog()method provides a controlled way to terminate the watchdog thread in tests, helping prevent resource leaks. The@VisibleForTestingannotation correctly signals that this is a testing-only API.server/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/DataIndexer.java (2)
27-30: LGTM - Explicit imports preferred.Replacing wildcard imports with explicit imports improves code clarity.
149-152: LGTM - Proper watchdog shutdown implementation.The method correctly closes the
indexerRefresherto terminate the background thread. This is the implementation thatAbstractDataManager.shutdownWatchDog()delegates to.server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (2)
27-27: LGTM - Import additions support test cleanup.Import reorganization to support the new
@Aftercleanup method.Also applies to: 33-34
42-42: LGTM - Proper test lifecycle management.The combination of reinitializing
interestsin@Beforeand shutting down the watchdog in@Afterensures proper test isolation and prevents resource leaks from background threads.Also applies to: 46-50
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/DataCacheTest.java (2)
32-33: LGTM - Import organization improved.Import reordering and addition of static Mockito imports support the refactored test structure.
Also applies to: 42-42
56-94: LGTM - Consistent resource cleanup pattern across all tests.All tests now follow a defensive try-finally pattern:
- Initialize fixtures to
nulloutside the try block- Execute test logic inside try
- Ensure
shutdownWatchDog()is called in finally when fixtures are not nullThis prevents watchdog thread leaks even if assertions fail or exceptions occur during tests, directly addressing the PR's memory leak concerns.
Also applies to: 99-146, 151-171, 176-207, 212-235, 310-384, 389-469, 474-541, 546-601
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.java (4)
34-35: Minor import formatting.The explicit import of
Listis a minor organizational change with no functional impact.
39-41: Good resource management pattern.The introduction of the try-finally structure with null-initialized variables enables deterministic cleanup of test resources. This directly addresses the memory leak concerns mentioned in the PR objectives.
42-76: Well-structured test with proper mocking.The test setup correctly:
- Instantiates
SessionInterestsandSessionWatchersas real objects (enabling proper cleanup)- Mocks external dependencies (
DataStore,Exchange,Server)- Verifies connection management behavior for both matching and non-matching IPs
- Uses appropriate test utilities from
TestUtilsfor channel creation
77-85: Cleanup pattern is correct and safe.The finally block properly ensures watchdog shutdown for both
SessionInterestsandSessionWatchers. The null checks are appropriately defensive. SinceshutdownWatchDog()is idempotent and does not throw exceptions (it only sets an internal flag to close the background thread), the cleanup will reliably execute for both resources without need for additional exception wrapping.server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java (1)
40-134: Excellent resource cleanup pattern!The try-finally block ensures
shutdownWatchDog()is always called, preventing watchdog thread leaks even when tests fail. This directly addresses the PR objective of fixing memory leaks in the unit test pipeline.
| pubExecutor.shutdown(); | ||
| unpubExecutor.shutdown(); | ||
| clientOffExecutor.shutdown(); | ||
| refreshTimer.cancel(); |
There was a problem hiding this comment.
Move executor and timer cleanup to finally block.
The pubExecutor, unpubExecutor, clientOffExecutor, and refreshTimer are currently shut down in the try block. If any assertion fails (lines 237-249), these resources will leak—directly contradicting the PR's goal of fixing memory leaks in the test pipeline.
🔎 Proposed fix to ensure cleanup in finally block
Assert.assertTrue(store.getConnectIds().size() >= urls.size() - clientOffs.size());
- pubExecutor.shutdown();
- unpubExecutor.shutdown();
- clientOffExecutor.shutdown();
- refreshTimer.cancel();
} finally {
+ if (pubExecutor != null) {
+ pubExecutor.shutdown();
+ }
+ if (unpubExecutor != null) {
+ unpubExecutor.shutdown();
+ }
+ if (clientOffExecutor != null) {
+ clientOffExecutor.shutdown();
+ }
+ if (refreshTimer != null) {
+ refreshTimer.cancel();
+ }
if (storeInstance != null) {
storeInstance.shutdownWatchDog();
}
}📝 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.
| pubExecutor.shutdown(); | |
| unpubExecutor.shutdown(); | |
| clientOffExecutor.shutdown(); | |
| refreshTimer.cancel(); | |
| Assert.assertTrue(store.getConnectIds().size() >= urls.size() - clientOffs.size()); | |
| } finally { | |
| if (pubExecutor != null) { | |
| pubExecutor.shutdown(); | |
| } | |
| if (unpubExecutor != null) { | |
| unpubExecutor.shutdown(); | |
| } | |
| if (clientOffExecutor != null) { | |
| clientOffExecutor.shutdown(); | |
| } | |
| if (refreshTimer != null) { | |
| refreshTimer.cancel(); | |
| } | |
| if (storeInstance != null) { | |
| storeInstance.shutdownWatchDog(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java
around lines 251-254, Move the shutdown/cleanup calls for pubExecutor,
unpubExecutor, clientOffExecutor, and refreshTimer out of the try block and into
a finally block so they always run even if assertions fail; locate the test
method in SessionDataStoreTest where pubExecutor.shutdown(),
unpubExecutor.shutdown(), clientOffExecutor.shutdown(), and
refreshTimer.cancel() are currently invoked and wrap the try { ... } with a
finally { pubExecutor.shutdown(); unpubExecutor.shutdown();
clientOffExecutor.shutdown(); refreshTimer.cancel(); } to guarantee resource
cleanup.
b3d3e98 to
b333172
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In
@server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java:
- Line 153: Hardcoded path separator used when building dumpPath can fail on
Windows; replace the string concatenation that sets dumpPath using "/" with a
platform-independent construction via java.nio.file.Paths (e.g.,
Paths.get(dumpDir, gcBeanName + ".hprof").toString()) and update any subsequent
usage in generateHeapDump to accept the new string path returned by Paths.get,
ensuring dumpDir, gcBeanName, and generateHeapDump are referenced unchanged.
♻️ Duplicate comments (5)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java (1)
251-254: Executor and timer cleanup still not in finally block.The executors and timer remain in the try block rather than the finally block. If any of the assertions (lines 237-249) fail, these resources will leak, which contradicts the PR's goal of fixing memory leaks in the test pipeline.
As noted in the previous review, these shutdown calls should be moved to the finally block with null checks to guarantee cleanup.
🔎 Proposed fix (from previous review)
Assert.assertTrue(store.getConnectIds().size() >= urls.size() - clientOffs.size()); - pubExecutor.shutdown(); - unpubExecutor.shutdown(); - clientOffExecutor.shutdown(); - refreshTimer.cancel(); } finally { + if (pubExecutor != null) { + pubExecutor.shutdown(); + } + if (unpubExecutor != null) { + unpubExecutor.shutdown(); + } + if (clientOffExecutor != null) { + clientOffExecutor.shutdown(); + } + if (refreshTimer != null) { + refreshTimer.cancel(); + } if (storeInstance != null) { storeInstance.shutdownWatchDog(); } }server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (4)
50-54: Critical bugs from previous review remain unaddressed.The following critical issues were previously identified but not fixed:
runningflag is never set totrue(see line 90 instartMonitoring()) - causingstopMonitoring()to always return earlylockshould befinal- it's initialized once and never reassigned- Typo:
monitoryThread→monitorThreadrunningshould bevolatilefor cross-thread visibility🔎 Proposed fix
- private Lock lock; + private final Lock lock; - private boolean running; + private volatile boolean running; - private Thread monitoryThread; + private Thread monitorThread;Then in
startMonitoring()(line 90):LOGGER.error("[UnitTestGCMonitor] 开始监控 GC 情况"); + this.running = true; - Thread monitoryThread = new Thread(this, "UnitTestGCMonitorThread"); + Thread monitorThread = new Thread(this, "UnitTestGCMonitorThread"); - monitoryThread.start(); + monitorThread.start(); - this.monitoryThread = monitoryThread; + this.monitorThread = monitorThread;And in
stopMonitoring()(line 110-111):- this.monitoryThread.interrupt(); - this.monitoryThread = null; + this.monitorThread.interrupt(); + this.monitorThread = null; + this.running = false;
79-79: CRITICAL: Diamond operator breaks Java 1.6 compilation.The diamond operator
<>requires Java 7+, but the project compiles with-source 1.6, causing the unit test pipeline to fail. This directly blocks the PR objective of debugging the unit test pipeline.🔎 Proposed fix
- this.countMap = new HashMap<>(); + this.countMap = new HashMap<String, Long>();
182-185: Major issue: Interrupt doesn't exit the monitoring loop.After detecting the interrupt, the code logs but continues the
while(true)loop instead of exiting. The monitoring thread won't stop gracefully.🔎 Proposed fix
if (Thread.interrupted()) { // 响应中断 LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程被中断，退出!"); + return; }
187-189: Exception logging loses stack trace.String concatenation with
Throwableonly capturestoString(), losing the full stack trace needed for debugging.🔎 Proposed fix
} catch (Throwable throwable) { - LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出" + throwable); + LOGGER.error("[UnitTestGCMonitor] 监控 GC 线程执行异常，退出", throwable); }
🧹 Nitpick comments (4)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.java (2)
54-76: Test logic is sound and comprehensive.The test properly verifies ConnectionsService behavior with appropriate mocking and assertions. Pre-condition checks and post-setup assertions provide good coverage.
Consider splitting this into separate test methods for improved clarity and maintainability:
testGetConnectionsReturnsEmptyWhenNoChannels()testGetConnectionsReturnsRemoteAddresses()testGetIpConnectsFiltersById()
37-38: Consider a more descriptive test method name.The generic name
test()doesn't convey what behavior is being verified. A more descriptive name would improve test readability and make failures easier to understand.🔎 Suggested naming
- public void test() { + public void testConnectionServiceReturnsConnectedChannels() {Alternative names:
testGetConnectionsAndIpConnectsWithMockedChannels()ortestConnectionManagementWithRemoteChannels()server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (2)
32-37: Consider using the project's custom Logger wrapper.This file imports
org.slf4j.Loggerdirectly, while the rest of the codebase uses a custom wrapper (com.alipay.sofa.registry.log.LoggerandLoggerFactory). While this may be intentional for test utilities, consistency would improve maintainability.🔎 Proposed change
-import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import com.alipay.sofa.registry.log.Logger; +import com.alipay.sofa.registry.log.LoggerFactory;
67-67: Hardcoded/tmppath may fail on Windows.The default dump directory
/tmpdoesn't exist on Windows systems, which could cause issues if tests run on Windows CI or developer machines.🔎 Proposed improvement
Use a platform-independent temp directory:
if (null == INSTANCE) { - INSTANCE = new UnitTestGCMonitor("/tmp", 3, 10, TimeUnit.SECONDS); + String tempDir = System.getProperty("java.io.tmpdir"); + INSTANCE = new UnitTestGCMonitor(tempDir, 3, 10, TimeUnit.SECONDS); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.javaserver/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffAddressService.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/AbstractDataManager.javaserver/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/DataIndexer.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/DataCacheTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.javaserver/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.javaserver/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchPersistenceSystemProperty.javaserver/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchSystemPropertyService.java
💤 Files with no reviewable changes (1)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java
🚧 Files skipped from review as they are similar to previous changes (7)
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/converter/pb/BoltEncodeTest.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java
- server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchPersistenceSystemProperty.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffServiceTest.java
🧰 Additional context used
🧬 Code graph analysis (6)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java (4)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/ConnectId.java (1)
ConnectId(31-151)server/common/util/src/main/java/com/alipay/sofa/registry/util/StringFormatter.java (1)
StringFormatter(22-29)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/PublisherUtils.java (1)
PublisherUtils(36-119)server/common/util/src/main/java/com/alipay/sofa/registry/util/ConcurrentUtils.java (1)
ConcurrentUtils(34-122)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffAddressService.java (3)
server/common/util/src/main/java/com/alipay/sofa/registry/util/CollectionUtils.java (1)
CollectionUtils(31-76)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1)
ValueConstants(26-200)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/registry/ClientManagerMetric.java (1)
ClientManagerMetric(27-58)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.java (2)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/TestUtils.java (1)
TestUtils(66-502)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/ConnectId.java (1)
ConnectId(31-151)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java (1)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
LoggerFactory(23-66)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/DataCacheTest.java (5)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/DataInfo.java (1)
DataInfo(29-180)server/common/util/src/main/java/com/alipay/sofa/registry/net/NetUtil.java (1)
NetUtil(42-228)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/URL.java (1)
URL(32-244)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1)
ValueConstants(26-200)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/BaseInfo.java (1)
BaseInfo(33-448)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java (3)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java (1)
Path(34-56)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/BlacklistDataResource.java (1)
Path(40-107)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MetaDigestResource.java (1)
Path(47-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (15)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/DataCacheTest.java (2)
32-33: LGTM: Import reorganization improves clarity.The explicit imports and grouping of static imports at the end follow common Java conventions.
Also applies to: 41-42
54-95: Excellent resource management refactoring.All test methods now use try-finally blocks with lazy initialization and null-safe
shutdownWatchDog()cleanup. This directly addresses the memory leak issue mentioned in the commit message and ensures deterministic resource cleanup even when tests fail.The consistent pattern across all nine test methods improves maintainability and reliability.
Also applies to: 97-147, 149-172, 174-208, 210-236, 308-385, 387-470, 472-542, 544-602
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionDataStoreTest.java (1)
38-135: Watchdog cleanup properly implemented.The try/finally pattern ensures the watchdog is shut down even if assertions fail, directly addressing the memory leak concerns outlined in the PR objectives.
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/connections/ConnectionsServiceTest.java (4)
34-34: LGTM!The
Listimport is necessary for the type declarations used in the test method assertions.
39-41: Excellent resource management pattern.Declaring the resources outside the try block and initializing them to null enables proper cleanup in the finally block, preventing watchdog thread leaks. This aligns well with the PR objective of fixing memory leaks in the test pipeline.
48-52: LGTM!The SessionInterests and SessionWatchers instances are properly initialized and assigned. The subsequent cleanup in the finally block ensures these resources won't leak.
77-85: Excellent cleanup implementation.The finally block with null checks ensures watchdog threads are properly shut down even if the test fails partway through, directly addressing the memory leak concerns mentioned in the PR objectives.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/DataIndexer.java (2)
26-30: LGTM: Import cleanup improves code clarity.Replacing the wildcard import with explicit imports follows Java best practices and makes dependencies more transparent.
148-152: Thread-safe shutdown of IndexerRefresher verified.The
WakeUpLoopRunnable.close()method is thread-safe and idempotent. It sets a volatileclosedflag that the loop checks at each iteration (line 65 of LoopRunnable), enabling graceful shutdown. Tests can safely call this method while the refresh loop is running; multiple calls are safe with no side effects.server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/providedata/AbstractFetchSystemPropertyService.java (1)
168-172: LGTM: Consistent testing hook for watchdog cleanup.The method provides a clean way for tests to terminate the background fetch thread, preventing resource leaks. This aligns with the PR objective to fix memory leaks in the unit test pipeline.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/store/AbstractDataManager.java (2)
25-25: LGTM: Import added for new annotation.
258-262: LGTM: Proper delegation to indexer shutdown.The method correctly delegates to the underlying
DataIndexer.shutdownWatchDog(), maintaining proper encapsulation while exposing the necessary testing hook.server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchClientOffAddressService.java (2)
44-57: LGTM: Import reorganization follows Java conventions.Moving static imports after regular imports aligns with standard Java style guidelines.
390-398: LoopRunnable.close() is thread-safe and properly handles concurrent calls.Verified that
LoopRunnable.close()implementation uses a volatile boolean flag (closed = true), which provides safe visibility across threads without requiring explicit synchronization. This idempotent operation safely handles concurrent calls. BothClientManagerProcessorandClientOpenFailWatchDogextendLoopRunnableand do not overrideclose(). The shutdown sequence correctly callssuper.shutdownWatchDog()before closing child processors, allowing the parent to perform necessary cleanup first while the volatile flag ensures the run loop exits gracefully.server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java (1)
44-50: LGTM! Import consolidation is clean.The added imports consolidate existing usage throughout the class (e.g.,
@Resourceat lines 75, 97;PathandProviderat lines 359-360). No functional changes introduced.
| continue; | ||
| } | ||
|
|
||
| String dumpPath = this.dumpDir + "/" + gcBeanName + ".hprof"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use platform-independent path separator.
Hardcoded "/" fails on Windows. Since the code already uses java.nio.file.Paths, leverage it consistently.
🔎 Proposed fix
- String dumpPath = this.dumpDir + "/" + gcBeanName + ".hprof";
+ Path dumpPath = Paths.get(this.dumpDir, gcBeanName + ".hprof");Then update subsequent usage:
- if (Files.exists(Paths.get(dumpPath))) {
+ if (Files.exists(dumpPath)) {
// ...
- dumpPath);
+ dumpPath.toString());And in generateHeapDump:
- private boolean generateHeapDump(String dumpPath) {
+ private boolean generateHeapDump(Path dumpPath) {
// ...
- Path filePath = Paths.get(dumpPath);
+ Path filePath = dumpPath;
// ...
- hotSpotBean.dumpHeap(dumpPath, true);
+ hotSpotBean.dumpHeap(dumpPath.toString(), true);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@server/server/session/src/test/java/com/alipay/sofa/registry/server/session/utils/UnitTestGCMonitor.java
around line 153, Hardcoded path separator used when building dumpPath can fail
on Windows; replace the string concatenation that sets dumpPath using "/" with a
platform-independent construction via java.nio.file.Paths (e.g.,
Paths.get(dumpDir, gcBeanName + ".hprof").toString()) and update any subsequent
usage in generateHeapDump to accept the new string path returned by Paths.get,
ensuring dumpDir, gcBeanName, and generateHeapDump are referenced unchanged.
1 participant
验证下 unit test pipeline 是否能正常工作，如果不能再考虑修改下配置
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.