add stop biz state #1026
add stop biz state #1026
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java (2)
79-81: Fix formatting in else conditionThe STOPPED state handling is correctly implemented, but there's a minor formatting issue.
Apply this formatting fix:
- }else { + } else {
46-53: Consider updating lifecycle documentationThe addition of the STOPPED state and refinement of BROKEN state represents a significant enhancement to the biz lifecycle management. Consider updating the overall lifecycle documentation to reflect these changes, particularly the distinction between STOPPED, BROKEN, and DEACTIVATED states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pom.xml(1 hunks)sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java(2 hunks)
🔇 Additional comments (6)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java (2)
46-48: LGTM: Improved clarity in BROKEN state documentation
The expanded comment better describes the scenarios where BROKEN state occurs, now correctly including both installation and uninstallation failures.
50-53: LGTM: Well-documented new state with verification needed
The new STOPPED state is a good addition that clearly distinguishes between successful and failed uninstallation states.
Let's verify that all switch statements handling BizState are updated:
✅ Verification successful
Based on the extensive search results, I can now make a determination about the impact of adding the STOPPED state. The codebase shows that:
- The BizState enum is used extensively throughout the codebase for state management
- All switch statements and if-else chains handling BizState are properly implemented using state-specific logic rather than exhaustive state matching
- The key state transition logic in BizModel.stop() already handles the transition to STOPPED state appropriately
- The state verification in BizManagerService and other components is based on specific state checks rather than exhaustive state matching
LGTM: The STOPPED state addition is safe and properly integrated
The new STOPPED state is well-integrated into the existing state machine design. The codebase uses targeted state checks (e.g., BizState.ACTIVATED, BizState.DEACTIVATED, etc.) rather than exhaustive switch statements, making it resilient to state additions. The state transitions in BizModel.stop() already handle the transition to STOPPED state correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for switch statements or if-else chains handling BizState
# to ensure they account for the new STOPPED state
# Look for switch statements on BizState
ast-grep --pattern 'switch ($expr) { $$$ }' | rg -A 5 'BizState'
# Look for if-else chains comparing BizState
rg -l "if.*getState\(\).*==.*BizState\."
Length of output: 107
Script:
#!/bin/bash
# Let's try a more comprehensive search for BizState usage patterns
# Search for BizState enum usage with context
rg -A 5 -B 5 "BizState\."
# Search for getState() method calls with context
rg -A 5 -B 5 "getState\(\)"
# Search for state transitions or comparisons
rg -A 5 -B 5 "(setState|getState|BizState)"
# Look for files containing BizState references
rg --files-with-matches "BizState"
Length of output: 515935
pom.xml (2)
17-17: Version update aligns with new feature development.
The version update from 2.2.15 to 2.2.16-SNAPSHOT is appropriate for the development of the new business state feature.
Line range hint 28-28: Fix typo in maven.gpg.plugin property name.
The property maven.gpg.pluign is misspelled and should be corrected to maven.gpg.plugin.
This property is referenced in multiple profiles. Apply these changes:
- Fix the property name:
- <maven.gpg.pluign>1.6</maven.gpg.pluign>
+ <maven.gpg.plugin>1.6</maven.gpg.plugin>- Update references in the release profile:
- <version>${maven.gpg.pluign}</version>
+ <version>${maven.gpg.plugin}</version>- Update references in the snapshot profile:
- <version>${maven.gpg.pluign}</version>
+ <version>${maven.gpg.plugin}</version>✅ Verification successful
Fix typo in maven.gpg.plugin property name at three locations in pom.xml
The verification confirms the presence of the misspelled property maven.gpg.pluign at exactly three locations in pom.xml:
- Property definition:
<maven.gpg.pluign>1.6</maven.gpg.pluign> - Two version references:
<version>${maven.gpg.pluign}</version>
The original review comment correctly identified all instances that need to be fixed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all references to the GPG plugin property
# Test: Search for any remaining instances of the misspelled property
rg "maven\.gpg\.pluign"
Length of output: 224
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (2)
424-424: LGTM! State transition improvement
Changing to STOPPED state is more semantically accurate than UNRESOLVED as it better represents the final state of a biz that has been deliberately stopped and cleaned up.
424-424: Verify state handling across the codebase
Please ensure that all code paths handling BizState are updated to handle the new STOPPED state appropriately.
Let's verify the state handling:
✅ Verification successful
Based on the search results, I can now provide a final response regarding the state handling verification:
BizState handling is properly implemented across the codebase
The codebase consistently handles BizState transitions through well-defined checks and state management. The STOPPED state is not referenced in any conditional checks or state comparisons, indicating it's likely a transitional state that doesn't require explicit handling in business logic. The existing code paths primarily focus on:
- State transitions between
ACTIVATEDandDEACTIVATED - Validation checks for
RESOLVEDstate before operations - Error handling for
BROKENstate
The addition of STOPPED state in setBizState() is safe as it's used as an internal transition state and doesn't impact the core state handling logic across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for BizState usage and potential missing handlers
# Test 1: Find all switch/if statements handling BizState
echo "=== Checking BizState handling ==="
rg -A 5 "switch.*BizState|if.*getBizState|if.*bizState"
# Test 2: Find all direct BizState comparisons
echo "=== Checking BizState comparisons ==="
rg "== BizState\.|equals.*BizState\."
# Test 3: Find potential monitoring/logging of biz states
echo "=== Checking state monitoring/logging ==="
rg "getState|getBizState.*log|monitor.*state"
Length of output: 15645
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1026 +/- ##
============================================
+ Coverage 78.22% 78.34% +0.12%
Complexity 874 874
============================================
Files 171 171
Lines 7053 7056 +3
Branches 1036 1037 +1
============================================
+ Hits 5517 5528 +11
+ Misses 1005 995 -10
- Partials 531 533 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java (1)
27-27: Clarification needed for UNRESOLVED state description.The updated comment "not init or not start install yet" could be more precise. Consider rephrasing to clearly distinguish between initialization and installation states.
- * not init or not start install yet + * not initialized or installation not yet startedsofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java (1)
Line range hint
147-205: Consider adding tests for edge cases.While the current tests cover the main scenarios, consider adding tests for:
- Multiple stop calls on the same biz
- Stop call on an already stopped biz
- Stop call during state transition
Would you like me to help implement these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pom.xml(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java(3 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🧰 Additional context used
🪛 GitHub Check: codecov/patch
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java
[warning] 80-80: sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java#L80
Added line #L80 was not covered by tests
🔇 Additional comments (4)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java (2)
46-48: Improved clarity in BROKEN state description.
The updated comment better reflects that failures can occur during both installation and uninstallation phases.
50-53: Verify state transition handling for new STOPPED state.
The addition of STOPPED state looks good, but we should ensure all possible transitions to/from this state are properly handled throughout the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response about the STOPPED state handling:
The STOPPED state is properly integrated into the state transition system.
The codebase shows proper handling of the STOPPED state:
- In BizModel.java, the state is set to STOPPED after successful uninstallation with proper cleanup:
setBizState(BizState.STOPPED, StateChangeReason.STOPPED) - State transition validation exists in BizModel.stop() method, which enforces valid transitions from ACTIVATED, DEACTIVATED, or BROKEN states
- BizManagerServiceImpl properly handles state transitions and validates states during operations
- Test coverage exists in BizModelTest.java verifying transitions to STOPPED state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper handling of STOPPED state transitions
# Look for state transition logic in the codebase
# Search for direct state assignments to STOPPED
rg -l "setState.*STOPPED"
# Search for potential transition handling code
rg -l "getState.*STOPPED"
# Look for switch/if-else statements handling states
ast-grep --pattern 'if ($expr) {
$$$
BizState.$$$
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search to find state transition handling
# Find all files referencing BizState enum
rg -l "BizState"
# Look for state transition methods and handlers
rg -A 5 "setState|getState"
# Search for switch/case statements handling BizState
ast-grep --pattern 'switch ($expr) {
$$$
case BizState.$$$:
$$$
}'
# Look for if-else chains with BizState comparisons
rg -A 3 "if.*getState\(\).*=.*BizState"
Length of output: 11159
Script:
#!/bin/bash
# Let's examine the implementation files more closely
# Check BizModel implementation for state handling
rg -A 10 "setState|getState" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
# Check BizManagerServiceImpl for state transitions
rg -A 10 "setState|getState" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java
# Look for state transition tests
rg -A 10 "setState|getState" sofa-ark-parent/core/spi/src/test/java/com/alipay/sofa/ark/spi/model/BizOperationTest.java
# Check DefaultBizDeployer for state handling during deployment
rg -A 10 "setState|getState" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/DefaultBizDeployer.java
Length of output: 549
Script:
#!/bin/bash
# Let's try a different approach to find state handling
# Look for BizState enum usage in implementation files
rg -A 5 "BizState\." sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/
# Check test files for state transition coverage
rg -A 5 "BizState\." sofa-ark-parent/core-impl/container/src/test/
# Look for uninstall/stop related code that might use STOPPED state
rg -A 5 "uninstall|stop" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/
# Check for state validation or transition methods
rg -A 5 "validateState|transition" sofa-ark-parent/core-impl/container/
Length of output: 73999
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java (2)
189-189: LGTM! State transition is correctly tested.
The test properly verifies that a successful stop operation transitions the biz to the STOPPED state when cleanup is enabled.
148-148: LGTM! Verify consistent state handling across the codebase.
The change from UNRESOLVED to STOPPED state provides better clarity about the biz lifecycle. The test correctly verifies this behavior when stop fails with cleanup enabled.
Let's verify consistent state handling across the codebase:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the state transition to STOPPED. The codebase shows that:
- The BizModel.stop() implementation correctly transitions to STOPPED state when cleanup is enabled (default behavior)
- The state transition is consistent across the codebase with proper test coverage
- No other components expect UNRESOLVED state after stop operations
The change from UNRESOLVED to STOPPED state is correct and properly implemented.
The evidence shows:
- BizModel.stop() explicitly sets state to STOPPED via
setBizState(BizState.STOPPED, StateChangeReason.STOPPED) - Test cases verify the STOPPED state transition in BizModelTest.java
- No code relies on UNRESOLVED state after stop operations
- The change provides better lifecycle clarity by distinguishing between initial UNRESOLVED and post-stop STOPPED states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining UNRESOLVED state usage after stop operations
# Expected: No occurrences of setting state to UNRESOLVED after stop
# Search for state transitions in stop-related methods
ast-grep --pattern 'setBizState(BizState.UNRESOLVED)'
# Search for UNRESOLVED state assertions in stop-related tests
rg 'assertEquals.*BizState\.UNRESOLVED.*getBizState\(\).*stop'
Length of output: 120
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Find all BizState enum usages
rg "BizState\." -A 2 -B 2
# 2. Find all state transition patterns in stop-related methods
ast-grep --pattern 'stop() {
$$$
}'
# 3. Find test assertions around stop operations
rg "assertEquals.*getBizState\(\)" -A 2 -B 2
# 4. Look for setBizState calls
ast-grep --pattern 'setBizState($_)'
Length of output: 98401
| } else if (STOPPED.name().equalsIgnoreCase(state)) { | ||
| return STOPPED; |
There was a problem hiding this comment.
Add test coverage for STOPPED state handling.
The static analysis indicates that the STOPPED state handling in the of() method lacks test coverage. This is a critical path that should be tested.
Would you like me to help generate test cases for the following scenarios?
- Converting string "STOPPED" to BizState.STOPPED
- Case-insensitive handling ("stopped", "STOPPED", "Stopped")
- State transition to STOPPED
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-80: sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizState.java#L80
Added line #L80 was not covered by tests
| assertEquals(BizState.STOPPED, bizModel.getBizState()); | ||
| } finally { | ||
| ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true"); | ||
| } |
There was a problem hiding this comment.
Fix incorrect constant in cleanup.
The test assertion for the STOPPED state is correct. However, there's a mismatch in the cleanup:
- The try block sets
REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED - But the finally block resets
AUTO_UNINSTALL_WHEN_FAILED_ENABLE
This could lead to test pollution affecting other tests.
Apply this fix:
} finally {
- ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true");
+ ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true");
}📝 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.
| assertEquals(BizState.STOPPED, bizModel.getBizState()); | |
| } finally { | |
| ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true"); | |
| } | |
| assertEquals(BizState.STOPPED, bizModel.getBizState()); | |
| } finally { | |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true"); | |
| } |
Summary by CodeRabbit
STOPPEDstate in the business model to indicate a successful uninstallation.STOPPEDcondition accurately, enhancing lifecycle control.UNRESOLVEDandBROKENstates to clarify their meanings and scenarios.