enable handle AfterBizStopEvent #1011
Conversation
WalkthroughThis pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
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: 4
Outside diff range and nitpick comments (6)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/AfterBizStopFailedEvent.java (1)
23-26: Consider enhancing the class-level documentation.While the class implementation is correct, it would be beneficial to add more detailed documentation about the purpose of this event and when it's triggered. This would help developers better understand how and when to use or handle this event in their code.
Consider adding a more detailed class-level Javadoc comment, for example:
/** * Represents an event that is triggered after a business stop operation has failed. * This event contains information about the business that failed to stop and the exception that caused the failure. * * @author guolei.sgl (guolei.sgl@antfin.com) 2019/11/1 6:21 PM * @since [specify the version when this class was introduced] */sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizInfo.java (1)
188-193: Approved: Enhanced state change reasons with a minor suggestionThe addition of
INSTALL_FAILEDandUN_INSTALL_FAILEDenum values improves the specificity of state change reasons, which is a positive change. This enhancement allows for better differentiation between installation and uninstallation failures.For consistency, consider using "Install" instead of "Uninstall" in the string representation of
UN_INSTALL_FAILED:- UN_INSTALL_FAILED("Uninstall Failed"), + UN_INSTALL_FAILED("Un-install Failed"),This would make it more consistent with the
INSTALL_FAILEDrepresentation.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/event/EventAdminServiceImpl.java (1)
Line range hint
1-180: Summary: Event handling updated, consider reviewing entire event systemThe changes in this file look good and are consistently implemented. The shift from
BeforeBizRecycleEventtoAfterBizStopEventseems to be part of a larger refactoring effort to improve event handling in the framework.Given the nature of these changes, it would be beneficial to:
- Review the entire event handling system to ensure all related components have been updated consistently.
- Update any documentation related to the event lifecycle, especially regarding business recycling and stopping.
- Consider adding or updating unit tests to cover the new event handling logic.
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/event/EventTest.java (1)
Line range hint
1-350: Summary: Event renaming implemented consistently.The changes in this file consistently implement the renaming of
AfterBizFailedEventtoAfterBizStartupFailedEvent. This includes updates to the import statement, test method, and event handler class. The changes appear to be part of a larger refactoring effort to improve clarity in event naming within the sofa-ark framework.These modifications enhance the readability and maintainability of the code by providing a more specific and descriptive name for the event related to business startup failures.
Consider reviewing other parts of the codebase that might be affected by this event renaming, such as:
- Other test files that might use this event.
- Implementation classes that generate or handle this event.
- Documentation or comments that reference the old event name.
Ensuring consistency across the entire project will help maintain code quality and prevent potential bugs related to this change.
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (1)
216-218: Correct spelling of 'unInstall' to 'uninstall' in log messageThe word "unInstall" should be changed to "uninstall" to maintain consistent capitalization and standard spelling.
Apply this diff to correct the spelling:
- getLogger().error( - String.format("Start Biz: %s failed, try to unInstall this biz.", - biz.getIdentity())); + getLogger().error( + String.format("Start Biz: %s failed, try to uninstall this biz.", + biz.getIdentity()));sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (1)
391-391: InitializeisStopFailedcloser to its usage for clarityFor better code organization and readability, consider moving the initialization of
isStopFailedcloser to where it is used within thestopmethod.Move the declaration as follows:
-long start = System.currentTimeMillis(); +boolean isStopFailed = false; +long start = System.currentTimeMillis();This adjustment groups related variables together, enhancing code clarity.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- pom.xml (1 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (4 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/event/EventAdminServiceImpl.java (2 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/event/EventTest.java (3 hunks)
- sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (2 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (2 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/AfterBizStartupFailedEvent.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/AfterBizStopFailedEvent.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizInfo.java (1 hunks)
Additional comments not posted (16)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/AfterBizStartupFailedEvent.java (4)
29-29: Approve addition ofthrowablemember variable.The addition of the
throwablemember variable is a good practice. It allows the event to encapsulate information about the exception that caused the startup failure, which can be useful for error handling and logging.
31-35: Approve constructor update.The constructor has been correctly updated to initialize the new
throwablemember variable. The event topic remains unchanged, maintaining consistency with the existing implementation.
37-39: Approve addition ofgetThrowable()method.The new
getThrowable()method provides appropriate access to thethrowablemember variable. It follows the Java bean convention, which is good for consistency and compatibility with frameworks that may rely on this convention.
27-27: Approve class renaming and verify its impact.The renaming of
AfterBizFailedEventtoAfterBizStartupFailedEventimproves clarity and specificity. This change enhances code readability and maintainability.However, as this is a breaking change, please verify its impact on the codebase:
Verification successful
Class renaming verified successfully.
All references to
AfterBizFailedEventhave been updated toAfterBizStartupFailedEvent. No residual references to the old class name were found in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old class name # Test: Search for any remaining references to 'AfterBizFailedEvent' rg --type java 'AfterBizFailedEvent' # Test: Verify that all references now use 'AfterBizStartupFailedEvent' rg --type java 'AfterBizStartupFailedEvent'Length of output: 1535
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/AfterBizStopFailedEvent.java (3)
27-27: LGTM: Class declaration and purpose are clear and appropriate.The
AfterBizStopFailedEventclass extendsAbstractArkEvent<Biz>, which is consistent with the event pattern in the framework. The class name and package structure accurately reflect its purpose of representing a failed business stop event.
31-35: LGTM: Constructor implementation is correct and follows best practices.The constructor properly initializes the object state by calling the superclass constructor and setting the
throwablefield. Using a constant for the topic name (Constants.BIZ_EVENT_TOPIC_AFTER_INVOKE_BIZ_STOP_FAILED) is a good practice for maintainability.
37-39: LGTM: Getter method is correctly implemented.The
getThrowable()method follows Java conventions and provides appropriate access to the privatethrowablefield, maintaining good encapsulation.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/event/EventAdminServiceImpl.java (1)
22-22: LGTM: Import statement added correctlyThe new import for
AfterBizStopEventis correctly placed and aligns with the changes in thehandleEventmethod.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/event/EventTest.java (3)
34-34: LGTM: Import statement updated correctly.The import statement has been updated to reflect the new event class name
AfterBizStartupFailedEvent. This change is consistent with the renaming of the event class mentioned in the summary.
338-341: LGTM: Event handler updated correctly.The
BizFailedEventHandlerclass has been properly updated to handle the newAfterBizStartupFailedEvent. The method signature in thehandleEventmethod has been correctly modified to accept the new event type. The rest of the implementation remains unchanged, which is appropriate for this refactoring.
143-145: LGTM: Test method updated correctly. Verify event topic constant.The test method has been updated to use the new
AfterBizStartupFailedEventclass, which is consistent with the class renaming. The assertion still checks for the same event topic constantConstants.BIZ_EVENT_TOPIC_AFTER_BIZ_FAILED.Please verify that the
Constants.BIZ_EVENT_TOPIC_AFTER_BIZ_FAILEDconstant hasn't been renamed or updated elsewhere in the codebase to maintain consistency with the new event name. You can run the following script to check:Verification successful
Verified: The
BIZ_EVENT_TOPIC_AFTER_BIZ_FAILEDconstant remains unchanged and is consistently used across the codebase. No further action required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any changes to the BIZ_EVENT_TOPIC_AFTER_BIZ_FAILED constant # Search for the constant definition echo "Searching for BIZ_EVENT_TOPIC_AFTER_BIZ_FAILED constant definition:" rg --type java "BIZ_EVENT_TOPIC_AFTER_BIZ_FAILED\s*=\s*" -A 1 # Search for usages of the constant echo "\nSearching for usages of BIZ_EVENT_TOPIC_AFTER_BIZ_FAILED constant:" rg --type java "BIZ_EVENT_TOPIC_AFTER_BIZ_FAILED" -A 1Length of output: 1571
pom.xml (1)
17-18: LGTM! Version updates are consistent with development practices.The version updates look good:
sofa.ark.versionis incremented to 2.2.15-SNAPSHOT, indicating ongoing development.sofa.ark.version.oldis updated to 2.2.14, reflecting the previous stable release.These changes align with typical Maven version management practices for continuous integration and development.
To ensure version consistency across the project, please run the following script:
This script will help identify any places where version numbers might need to be updated to maintain consistency with these changes.
Verification successful
Version consistency verified successfully.
All version updates in
pom.xmlare correct, and no hardcoded references to2.2.13,2.2.14, or2.2.15-SNAPSHOTare found elsewhere in the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify version consistency across the project # Check for any hardcoded versions that might need updating echo "Checking for hardcoded versions:" rg --type xml "2\.2\.14" --glob "!pom.xml" # Verify that the new SNAPSHOT version is used consistently echo "Verifying SNAPSHOT version usage:" rg --type xml "2\.2\.15-SNAPSHOT" --glob "!pom.xml" # Check for any remaining references to the old version echo "Checking for old version references:" rg --type xml "2\.2\.13" --glob "!pom.xml"Length of output: 378
Script:
#!/bin/bash # Description: Comprehensive verification of version consistency across the project # Verify the updated versions in pom.xml echo "Verifying version updates in pom.xml:" grep -E "<sofa\.ark\.version>(2\.2\.15-SNAPSHOT)</sofa\.ark\.version>" pom.xml grep -E "<sofa\.ark\.version\.old>(2\.2\.14)</sofa\.ark\.version\.old>" pom.xml # Search for any remaining references to the old versions in all file types echo "Searching for references to version 2.2.13 and 2.2.14 across the codebase:" rg --glob "!pom.xml" --type-add 'all:any' -t all "(2\.2\.13|2\.2\.14)" # Confirm that the new SNAPSHOT version is not mistakenly hardcoded elsewhere echo "Ensuring no hardcoded references to 2.2.15-SNAPSHOT outside pom.xml:" rg --glob "!pom.xml" --type-add 'all:any' -t all "2\.2\.15-SNAPSHOT"Length of output: 934
Script:
#!/bin/bash # Description: Corrected comprehensive verification of version consistency across the project # Verify the updated versions in pom.xml echo "Verifying version updates in pom.xml:" grep -E "<sofa\.ark\.version>(2\.2\.15-SNAPSHOT)</sofa\.ark\.version>" pom.xml grep -E "<sofa\.ark\.version\.old>(2\.2\.14)</sofa\.ark\.version\.old>" pom.xml # Search for any remaining references to the old versions in all file types except pom.xml echo "Searching for references to version 2.2.13 and 2.2.14 across the codebase:" rg "(2\.2\.13|2\.2\.14)" --glob "!pom.xml" # Confirm that the new SNAPSHOT version is not mistakenly hardcoded elsewhere echo "Ensuring no hardcoded references to 2.2.15-SNAPSHOT outside pom.xml:" rg "2\.2\.15-SNAPSHOT" --glob "!pom.xml"Length of output: 762
Script:
#!/bin/bash # Description: Corrected comprehensive verification of version consistency across the project # Verify the updated versions in pom.xml echo "Verifying version updates in pom.xml:" grep -E "<sofa\.ark\.version>2\.2\.15-SNAPSHOT</sofa\.ark\.version>" pom.xml grep -E "<sofa\.ark\.version\.old>2\.2\.14</sofa\.ark\.version\.old>" pom.xml # Search for any remaining references to the old versions in all file types except pom.xml echo "Searching for references to version 2.2.13 and 2.2.14 across the codebase:" rg "(2\.2\.13|2\.2\.14)" -g "!pom.xml" # Confirm that the new SNAPSHOT version is not mistakenly hardcoded elsewhere echo "Ensuring no hardcoded references to 2.2.15-SNAPSHOT outside pom.xml:" rg "2\.2\.15-SNAPSHOT" -g "!pom.xml"Length of output: 750
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (3)
176-178: Approve the addition of the new constant and repositioning of the existing one.The addition of
BIZ_EVENT_TOPIC_AFTER_INVOKE_BIZ_STOP_FAILEDand the repositioning ofBIZ_EVENT_TOPIC_AFTER_INVOKE_BIZ_STOPimprove the event handling capabilities for biz stop operations. This change allows for more granular control and error handling in the biz lifecycle.
208-215: Approve the addition of the new configuration constant.The new constant
REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILEDprovides a configuration option to automatically remove a biz instance from the BizManagerService if it fails to stop. This addition enhances the framework's ability to handle failure scenarios and maintain system stability.
Line range hint
176-215: Summary: Enhancements to biz lifecycle event handling and failure managementThe changes in this file improve the SOFAArk framework's capabilities in two key areas:
- More granular event handling for biz stop operations, including a new event for failed stops.
- A new configuration option to automatically remove biz instances that fail to stop.
These additions enhance the framework's robustness in handling failure scenarios and provide more control over the biz lifecycle. The changes are well-structured and positioned logically within the existing constants.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (1)
345-346: Proper handling ofBizStateafter startup failureThe changes correctly set the
BizStatetoBROKENand send anAfterBizStartupFailedEventwhen an exception occurs during startup. This enhances error handling by accurately reflecting the failed state and notifying the system accordingly.
| if (event instanceof AfterBizStopEvent) { | ||
| classLoader = ((AfterBizStopEvent) event).getSource().getBizClassLoader(); |
There was a problem hiding this comment.
Tip
Codebase Verification
Issue: Residual References to BeforeBizRecycleEvent Found
The shell script identified remaining references to BeforeBizRecycleEvent in the following files:
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/BeforeBizRecycleEvent.javasofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.javasofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/event/EventTest.java
Please update these references to use AfterBizStopEvent to maintain consistency in event handling.
Analysis chain
LGTM: Event handling updated, please verify broader impact
The change from BeforeBizRecycleEvent to AfterBizStopEvent looks good and is consistent with the new import. The class loader retrieval has been appropriately updated to use getBizClassLoader().
However, this change might affect the timing of when the class loader is unregistered. Please ensure that this aligns with the intended behavior in the broader context of the framework.
To verify the consistency of this change across the codebase, please run the following script:
This will help ensure that all relevant parts of the codebase have been updated consistently.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to BeforeBizRecycleEvent and verify AfterBizStopEvent usage
echo "Checking for any remaining references to BeforeBizRecycleEvent:"
rg "BeforeBizRecycleEvent" --type java
echo "\nVerifying AfterBizStopEvent usage:"
rg "AfterBizStopEvent" --type java
Length of output: 4506
| if (!isStopFailed || (isStopFailed && removeInstanceAfterStopFailed)) { | ||
| BizManagerService bizManagerService = ArkServiceContainerHolder.getContainer() | ||
| .getService(BizManagerService.class); | ||
| bizManagerService.unRegisterBiz(bizName, bizVersion); | ||
| setBizState(BizState.UNRESOLVED, StateChangeReason.STOPPED); | ||
| eventAdminService.sendEvent(new BeforeBizRecycleEvent(this)); | ||
| urls = null; | ||
| denyImportPackages = null; | ||
| denyImportClasses = null; | ||
| denyImportResources = null; | ||
| // close classloader | ||
| if (classLoader instanceof AbstractClasspathClassLoader) { | ||
| try { | ||
| ((AbstractClasspathClassLoader) classLoader).close(); | ||
| ((AbstractClasspathClassLoader) classLoader).clearCache(); | ||
| } catch (IOException e) { | ||
| ArkLoggerFactory.getDefaultLogger().warn( | ||
| "Ark biz {} close biz classloader fail", getIdentity()); | ||
| } | ||
| } | ||
| classLoader = null; | ||
| recycleBizTempWorkDir(bizTempWorkDir); | ||
| bizTempWorkDir = null; | ||
| eventAdminService.sendEvent(new AfterBizStopEvent(this)); |
There was a problem hiding this comment.
Simplify conditional logic in resource cleanup
The condition in the finally block can be simplified for better readability:
if (!isStopFailed || (isStopFailed && removeInstanceAfterStopFailed)) {
// cleanup code
}Simplify the condition as follows:
-if (!isStopFailed || (isStopFailed && removeInstanceAfterStopFailed)) {
+if (!isStopFailed || removeInstanceAfterStopFailed) {
// cleanup code
}This refactoring retains the original logic while making the condition easier to understand.
Extract resource cleanup code into a separate method
The resource cleanup code in the finally block is extensive and impacts readability. To adhere to the Single Responsibility Principle and improve maintainability, consider extracting this code into a dedicated private method.
Create a new method:
private void cleanupResources() {
BizManagerService bizManagerService = ArkServiceContainerHolder.getContainer()
.getService(BizManagerService.class);
bizManagerService.unRegisterBiz(bizName, bizVersion);
setBizState(BizState.UNRESOLVED, StateChangeReason.STOPPED);
eventAdminService.sendEvent(new BeforeBizRecycleEvent(this));
urls = null;
denyImportPackages = null;
denyImportClasses = null;
denyImportResources = null;
// close classloader
if (classLoader instanceof AbstractClasspathClassLoader) {
try {
((AbstractClasspathClassLoader) classLoader).close();
((AbstractClasspathClassLoader) classLoader).clearCache();
} catch (IOException e) {
ArkLoggerFactory.getDefaultLogger().warn(
"Ark biz {} close biz classloader fail", getIdentity());
}
}
classLoader = null;
recycleBizTempWorkDir(bizTempWorkDir);
bizTempWorkDir = null;
eventAdminService.sendEvent(new AfterBizStopEvent(this));
}Then update the finally block:
if (!isStopFailed || removeInstanceAfterStopFailed) {
- // cleanup code
+ cleanupResources();
}This refactoring improves code organization and enhances readability.
| } catch (Throwable t) { | ||
| // handle stop failed | ||
| ArkLoggerFactory.getDefaultLogger().info("Ark biz {} stop failed in {} ms", | ||
| getIdentity(), (System.currentTimeMillis() - start)); | ||
| isStopFailed = true; | ||
| setBizState(BizState.BROKEN, StateChangeReason.UN_INSTALL_FAILED); | ||
| eventAdminService.sendEvent(new AfterBizStopFailedEvent(this, t)); | ||
| throw t; |
There was a problem hiding this comment.
Avoid rethrowing exceptions in stop method to ensure resource cleanup
Throwing an exception from the stop method's catch block may prevent the finally block from executing fully, which could leave resources improperly cleaned up. Consider logging the exception without rethrowing it to allow the finally block to complete and perform necessary cleanup operations.
Apply this diff to prevent rethrowing the exception:
} catch (Throwable t) {
// handle stop failed
ArkLoggerFactory.getDefaultLogger().info("Ark biz {} stop failed in {} ms",
getIdentity(), (System.currentTimeMillis() - start));
isStopFailed = true;
setBizState(BizState.BROKEN, StateChangeReason.UN_INSTALL_FAILED);
eventAdminService.sendEvent(new AfterBizStopFailedEvent(this, t));
- throw t;
}This change ensures that all cleanup steps in the finally block are executed, maintaining system stability.
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 t) { | |
| // handle stop failed | |
| ArkLoggerFactory.getDefaultLogger().info("Ark biz {} stop failed in {} ms", | |
| getIdentity(), (System.currentTimeMillis() - start)); | |
| isStopFailed = true; | |
| setBizState(BizState.BROKEN, StateChangeReason.UN_INSTALL_FAILED); | |
| eventAdminService.sendEvent(new AfterBizStopFailedEvent(this, t)); | |
| throw t; | |
| } catch (Throwable t) { | |
| // handle stop failed | |
| ArkLoggerFactory.getDefaultLogger().info("Ark biz {} stop failed in {} ms", | |
| getIdentity(), (System.currentTimeMillis() - start)); | |
| isStopFailed = true; | |
| setBizState(BizState.BROKEN, StateChangeReason.UN_INSTALL_FAILED); | |
| eventAdminService.sendEvent(new AfterBizStopFailedEvent(this, t)); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java (1)
179-190: Consider adding assertions to verify mock interactionsIn
testStopSucceedWithClean(), after callingbizModel.stop();, you verify theBizStatebut do not assert interactions with your mocks. Adding assertions to verify that certain methods were called can strengthen your tests by ensuring that the correct interactions occurred.For example, you could add:
verify(eventAdminService).sendEvent(any(BeforeBizStopEvent.class)); verify(bizManagerService).unregisterBiz(bizModel);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java (5 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (3 hunks)
Additional comments not posted (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
311-313: Improved test failure handling, but clarification needed on behavior changeThe changes improve the test failure handling by replacing
assertTrue(false)withfail(), which is more idiomatic and provides clearer error messages.However, the assertion change from
assertTruetoassertFalsealters the expected behavior of the test:-assertTrue(bizManagerServiceMock.getBiz("biz-install-failed-demo").isEmpty()); +assertFalse(bizManagerServiceMock.getBiz("biz-install-failed-demo").isEmpty());This change suggests that we now expect the failed biz to remain in the biz manager service after a failed installation, which is different from the previous expectation.
Could you please clarify if this change in expected behavior is intentional? If so, what prompted this change, and are there any related updates to the error handling or uninstallation process that we should be aware of?
To help verify this change, you can run the following script to check for any related changes in the error handling or uninstallation process:
This will help us understand if there have been any broader changes to the framework that justify this change in test behavior.
Also applies to: 326-328
| } catch (RuntimeException e) { | ||
| } |
There was a problem hiding this comment.
Avoid silently swallowing exceptions in tests
At lines 164-165, the RuntimeException caught is being suppressed without any handling or assertions. This practice can obscure test failures and hinder maintenance. Utilize assertThrows to assert that the exception is thrown, improving test reliability and readability.
Apply this change:
- try {
- bizModel.stop();
- } catch (RuntimeException e) {
- }
+ assertThrows(RuntimeException.class, () -> {
+ bizModel.stop();
+ });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 (RuntimeException e) { | |
| } | |
| assertThrows(RuntimeException.class, () -> { | |
| bizModel.stop(); | |
| }); |
| } catch (RuntimeException e) { | ||
| } |
There was a problem hiding this comment.
Avoid silently swallowing exceptions in tests
At lines 146-147, the RuntimeException caught in the catch block is swallowed silently. This can make debugging difficult if unexpected exceptions occur. Consider using assertThrows to verify that the exception is thrown as expected, which enhances test clarity and robustness.
Apply this change to use assertThrows:
- try {
- bizModel.stop();
- } catch (RuntimeException e) {
- }
+ assertThrows(RuntimeException.class, () -> {
+ bizModel.stop();
+ });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 (RuntimeException e) { | |
| } | |
| assertThrows(RuntimeException.class, () -> { | |
| bizModel.stop(); | |
| }); |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false"); | ||
| EventAdminService eventAdminService = mock(EventAdminService.class); | ||
| BizManagerService bizManagerService = mock(BizManagerService.class); | ||
|
|
||
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | ||
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | ||
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | ||
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | ||
| bizModel.stop(); | ||
|
|
||
| assertEquals(BizState.UNRESOLVED, bizModel.getBizState()); | ||
| } finally { | ||
| ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true"); | ||
| } |
There was a problem hiding this comment.
Incorrect configuration reset in finally block
At line 194, you set REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED to "false" for the test. However, in the finally block at line 206, you're resetting AUTO_UNINSTALL_WHEN_FAILED_ENABLE to "true", which is a different configuration property. This may lead to unintended side effects on other tests or behaviors.
Please correct the configuration reset to restore the original value of REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED. Consider storing the original value before changing it and then resetting it in the finally block:
+ String originalConfig = ArkConfigs.getStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED);
ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false");
EventAdminService eventAdminService = mock(EventAdminService.class);
BizManagerService bizManagerService = mock(BizManagerService.class);
ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class);
when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService);
when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService);
mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer);
bizModel.stop();
assertEquals(BizState.UNRESOLVED, bizModel.getBizState());
} finally {
- ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true");
+ ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, originalConfig);
}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.
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false"); | |
| EventAdminService eventAdminService = mock(EventAdminService.class); | |
| BizManagerService bizManagerService = mock(BizManagerService.class); | |
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | |
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | |
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | |
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | |
| bizModel.stop(); | |
| assertEquals(BizState.UNRESOLVED, bizModel.getBizState()); | |
| } finally { | |
| ArkConfigs.putStringValue(AUTO_UNINSTALL_WHEN_FAILED_ENABLE, "true"); | |
| } | |
| String originalConfig = ArkConfigs.getStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED); | |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false"); | |
| EventAdminService eventAdminService = mock(EventAdminService.class); | |
| BizManagerService bizManagerService = mock(BizManagerService.class); | |
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | |
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | |
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | |
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | |
| bizModel.stop(); | |
| assertEquals(BizState.UNRESOLVED, bizModel.getBizState()); | |
| } finally { | |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, originalConfig); | |
| } |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false"); | ||
| EventAdminService eventAdminService = mock(EventAdminServiceImpl.class); | ||
| BizManagerService bizManagerService = mock(BizManagerService.class); | ||
|
|
||
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | ||
| doThrow(new RuntimeException()).when(eventAdminService).sendEvent(any(BeforeBizStopEvent.class)); | ||
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | ||
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | ||
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | ||
| try { | ||
| bizModel.stop(); | ||
| } catch (RuntimeException e) { | ||
| } | ||
| assertEquals(BizState.BROKEN, bizModel.getBizState()); | ||
| } finally { | ||
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true"); | ||
| } |
There was a problem hiding this comment.
Ensure configuration properties are properly restored
In testStopFailedWithClean(), at line 153, you set REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED to "false". While you reset it to "true" in the finally block at line 168, it's safer to store the original configuration value before modifying it. This ensures that any existing configurations are not overwritten, preventing potential side effects on other tests.
Modify your code as follows:
+ String originalConfig = ArkConfigs.getStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED);
ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false");
EventAdminService eventAdminService = mock(EventAdminServiceImpl.class);
BizManagerService bizManagerService = mock(BizManagerService.class);
ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class);
doThrow(new RuntimeException()).when(eventAdminService).sendEvent(any(BeforeBizStopEvent.class));
when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService);
when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService);
mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer);
try {
bizModel.stop();
} catch (RuntimeException e) {
}
assertEquals(BizState.BROKEN, bizModel.getBizState());
} finally {
- ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true");
+ ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, originalConfig);
}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.
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false"); | |
| EventAdminService eventAdminService = mock(EventAdminServiceImpl.class); | |
| BizManagerService bizManagerService = mock(BizManagerService.class); | |
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | |
| doThrow(new RuntimeException()).when(eventAdminService).sendEvent(any(BeforeBizStopEvent.class)); | |
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | |
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | |
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | |
| try { | |
| bizModel.stop(); | |
| } catch (RuntimeException e) { | |
| } | |
| assertEquals(BizState.BROKEN, bizModel.getBizState()); | |
| } finally { | |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "true"); | |
| } | |
| String originalConfig = ArkConfigs.getStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED); | |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, "false"); | |
| EventAdminService eventAdminService = mock(EventAdminServiceImpl.class); | |
| BizManagerService bizManagerService = mock(BizManagerService.class); | |
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | |
| doThrow(new RuntimeException()).when(eventAdminService).sendEvent(any(BeforeBizStopEvent.class)); | |
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | |
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | |
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | |
| try { | |
| bizModel.stop(); | |
| } catch (RuntimeException e) { | |
| } | |
| assertEquals(BizState.BROKEN, bizModel.getBizState()); | |
| } finally { | |
| ArkConfigs.putStringValue(REMOVE_BIZ_INSTANCE_AFTER_STOP_FAILED, originalConfig); | |
| } |
| try (MockedStatic<ArkServiceContainerHolder> mockedStatic = Mockito.mockStatic(ArkServiceContainerHolder.class)) { | ||
| EventAdminService eventAdminService = mock(EventAdminServiceImpl.class); | ||
| BizManagerService bizManagerService = mock(BizManagerService.class); | ||
|
|
||
| ArkServiceContainer arkServiceContainer = mock(ArkServiceContainer.class); | ||
| doThrow(new RuntimeException()).when(eventAdminService).sendEvent(any(BeforeBizStopEvent.class)); | ||
| when(arkServiceContainer.getService(EventAdminService.class)).thenReturn(eventAdminService); | ||
| when(arkServiceContainer.getService(BizManagerService.class)).thenReturn(bizManagerService); | ||
| mockedStatic.when(ArkServiceContainerHolder::getContainer).thenReturn(arkServiceContainer); | ||
| try { | ||
| bizModel.stop(); | ||
| } catch (RuntimeException e) { | ||
| } | ||
| assertEquals(BizState.UNRESOLVED, bizModel.getBizState()); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
MockedStatic Usage Outside Try-With-Resources Detected
Several tests instantiate MockedStatic without using try-with-resources, which may cause side effects affecting other tests.
Affected files:
sofa-ark-parent/core/common/src/test/java/com/alipay/sofa/ark/common/util/ClassLoaderUtilTest.javasofa-ark-parent/support/ark-support-starter/src/test/java/com/alipay/sofa/ark/support/AddBizInResourcesHookTest.javasofa-ark-plugin/config-ark-plugin/src/test/java/com/alipay/sofa/ark/config/ApolloConfigActivatorTest.javasofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/model/BizModelTest.java
Analysis chain
Ensure proper use of MockedStatic to prevent side effects
When using MockedStatic with try-with-resources, it's important to ensure that it does not affect other tests. Confirm that no lingering effects from the static mocks remain after the test execution.
Run the following script to check for proper closure of MockedStatic mocks:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of MockedStatic outside try-with-resources blocks.
# Test: Find any MockedStatic instantiations not within try-with-resources.
# Expect: No occurrences found.
rg --type java 'MockedStatic<.*>.*Mockito\.mockStatic' -A 5 | rg -v 'try\s*\('
Length of output: 9787
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1011 +/- ##
============================================
+ Coverage 78.20% 78.41% +0.20%
- Complexity 873 874 +1
============================================
Files 170 172 +2
Lines 7033 7041 +8
Branches 1037 1037
============================================
+ Hits 5500 5521 +21
+ Misses 1003 991 -12
+ Partials 530 529 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/CleanAfterBizStopEvent.java (2)
23-26: LGTM: Well-structured class declaration. Consider enhancing the documentation.The class declaration is appropriate, extending
AbstractArkEvent<Biz>and following Java naming conventions. The documentation provides a basic explanation of the class's purpose.Consider expanding the class documentation to include:
- When this event is triggered in the lifecycle of a biz module.
- What specific cleanup actions are expected to be performed in response to this event.
- Any important considerations for handlers of this event.
Example:
/** * Represents an event triggered after a biz module has been stopped, signaling the need for cleanup. * This event is used to initiate the cleanup of the biz module and its associated event handlers * within the sofa-ark framework. * * Handlers of this event should perform final cleanup tasks such as: * - Releasing any remaining resources associated with the stopped biz * - Removing any lingering references to the biz module * - Performing any necessary state updates in the ark container * * Note: This event is triggered after the biz stop process has completed, so the biz * should no longer be in an active state when handling this event. */
27-30: LGTM: Well-implemented constructor. Consider adding a parameter check.The constructor correctly initializes the superclass and sets the appropriate topic for the event.
Consider adding a null check for the
sourceparameter to prevent potentialNullPointerExceptions:public CleanAfterBizStopEvent(Biz source) { super(Objects.requireNonNull(source, "Source Biz cannot be null")); this.topic = Constants.BIZ_EVENT_TOPIC_CLEAN_AFTER_INVOKE_BIZ_STOP; }Don't forget to add the corresponding import:
import java.util.Objects;sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/plugin/CleanAfterPluginStopEvent.java (2)
23-27: Enhance JavaDoc for better documentation.While the class declaration looks good, the JavaDoc could be improved:
- Add a brief description of the class's purpose and when it's triggered.
- Explain the significance of the "CleanAfter" prefix in the class name.
- Consider adding
@sincetag with the version number.Example:
/** * Event triggered after a plugin has been stopped, signaling the need for cleanup operations. * This event is part of the plugin lifecycle management in the SOFAArk framework. * * @author guolei.sgl (guolei.sgl@antfin.com) 2019/11/1 6:21 PM * @since 0.6.0 // Replace with the actual version number */
29-32: LGTM: Constructor is well-implemented. Consider adding a comment.The constructor correctly initializes the superclass and sets the appropriate topic. To improve readability, consider adding a brief comment explaining the significance of the
PLUGIN_EVENT_TOPIC_CLEAN_AFTER_INVOKE_PLUGIN_STOPconstant:public CleanAfterPluginStopEvent(Plugin source) { super(source); // Set the topic for cleanup operations after plugin stop this.topic = Constants.PLUGIN_EVENT_TOPIC_CLEAN_AFTER_INVOKE_PLUGIN_STOP; }sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/PluginModel.java (1)
371-371: LGTM: New CleanAfterPluginStopEvent dispatch addedThe addition of
CleanAfterPluginStopEventdispatch in thefinallyblock ensures it will be called regardless of exceptions during the stop process. The order of events (Before -> After -> Clean) is logical.Consider adding a comment explaining the purpose of
CleanAfterPluginStopEventand any specific cleanup operations it might trigger. This would help other developers understand the event's role in the plugin lifecycle.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (2 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/PluginModel.java (3 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/event/EventAdminServiceImpl.java (2 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/CleanAfterBizStopEvent.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/plugin/CleanAfterPluginStopEvent.java (1 hunks)
Files skipped from review due to trivial changes (1)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
Files skipped from review as they are similar to previous changes (3)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/event/EventAdminServiceImpl.java
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java
Additional comments not posted (8)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/biz/CleanAfterBizStopEvent.java (3)
1-16: LGTM: Proper license header included.The file includes the standard Apache License 2.0 header, which is appropriate for this project.
17-21: LGTM: Appropriate package and imports.The package declaration is consistent with the file path, and the imports are relevant to the class implementation. No unused imports are present.
1-31: Overall assessment: Well-implemented new event class.The
CleanAfterBizStopEventclass is a valuable addition to the sofa-ark framework, providing a clear mechanism for handling cleanup after a biz module is stopped. The implementation is concise, follows good coding practices, and integrates well with the existing architecture.Key points:
- The class extends the appropriate base class (
AbstractArkEvent<Biz>).- It uses a specific constant for the event topic, ensuring consistency across the framework.
- The constructor is simple and focused.
The suggested improvements (enhancing documentation and adding a null check) are minor and aimed at further increasing the robustness and clarity of the code.
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/event/plugin/CleanAfterPluginStopEvent.java (3)
1-17: LGTM: License and package declaration are correct.The Apache License 2.0 header is properly included, and the package name follows Java naming conventions.
19-21: LGTM: Imports are appropriate and concise.All imported classes are used in the implementation, and there are no unnecessary imports.
1-33: Overall assessment: Well-structured new event class with minor improvement suggestions.The
CleanAfterPluginStopEventclass is a well-implemented addition to the SOFAArk framework's plugin event system. It follows good coding practices and integrates seamlessly with the existing architecture. The suggested improvements are minor and mainly focus on enhancing documentation for better maintainability.To fully understand the impact of this new event class, it would be helpful to see how it's used in the broader context of the plugin lifecycle management. Consider providing additional context in the PR description about the motivation for adding this event and any related changes in other parts of the codebase.
To better understand the usage and impact of this new event class, let's search for its usage in the codebase:
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/PluginModel.java (2)
29-29: LGTM: New import statement for CleanAfterPluginStopEventThe import statement for
CleanAfterPluginStopEventis correctly placed with other event-related imports. This addition is necessary for using the new event in thestop()method.
Line range hint
1-380: Verify related classes and update documentationThe changes to
PluginModelappear to be part of a larger update to the plugin lifecycle management. To ensure consistency across the codebase:
- Verify if similar changes (addition of
CleanAfterPluginStopEvent) are needed in other related classes that handle plugin lifecycle.- Update any existing documentation (e.g., JavaDocs, README, or developer guides) that describe the plugin lifecycle to include information about the new
CleanAfterPluginStopEvent.To help verify the impact of these changes, you can run the following script:
fix koupleless/koupleless#256
Summary by CodeRabbit
New Features
Bug Fixes
Documentation