Module deployments are deduplicated when static merge deployments(#338) #1035
Conversation
|
Hi @Willhow-Gao, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
sofastack-cla
bot
added
question
WalkthroughThe pull request introduces modifications to the business entity registration logic within 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: 4
🧹 Outside diff range and nitpick comments (6)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizManagerService.java (2)
160-161: Add Javadoc documentation for the new method.The new
registerBizIfAbsentmethod lacks Javadoc documentation. Please add documentation following the interface's convention to describe:
- The method's purpose
- The
bizparameter- The return value semantics (when it returns true/false)
Consider this template:
+ /** + * Register Biz only if it's not already registered + * + * @param biz The business module to register + * @return true if the biz was registered, false if it already exists + */ boolean registerBizIfAbsent(Biz biz);
160-161: Consider reorganizing method placement.For better code organization and readability, consider moving this method next to the similar
registerBizmethod (around line 42), as they are functionally related.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java (1)
226-239: Document the thread-safety guarantees.The method implements thread-safe registration, but this behavior isn't documented. Consider adding Javadoc to clarify the concurrency guarantees.
Add documentation like this:
+ /** + * Thread-safe registration of a Biz instance if not already present. + * This method ensures that only one instance of a Biz with the same name and version + * can be registered, even under concurrent access. + * + * @param biz the Biz instance to register + * @return true if the Biz was registered, false if an instance was already present + * @throws IllegalArgumentException if biz is null or not in RESOLVED state + */ @Override public boolean registerBizIfAbsent(Biz biz) {sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java (2)
197-201: Consider adding logging for skipped registrations.To improve observability, consider adding debug-level logging when a biz registration is skipped due to it already being present.
public void processStaticBizFromClasspath(PipelineContext pipelineContext) throws Exception { ExecutableArchive executableArchive = pipelineContext.getExecutableArchive(); List<BizArchive> bizArchives = executableArchive.getBizArchives(); for (BizArchive bizArchive : bizArchives) { Biz biz = bizFactoryService.createBiz(bizArchive); - bizManagerService.registerBizIfAbsent(biz); + if (!bizManagerService.registerBizIfAbsent(biz)) { + ArkLoggerFactory.getDefaultLogger().debug( + "Skipped registering duplicate biz: {}", biz.getIdentity()); + } } }
197-201: Consider adding unit tests for deduplication.To ensure the deduplication behavior works as expected, consider adding unit tests that verify:
- First registration succeeds
- Duplicate registration is properly handled
- Error cases are properly handled
Would you like me to help generate the unit test cases?
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java (1)
Line range hint
196-202: Consider enhancing error reporting for duplicate biz detectionSince we're now explicitly handling duplicate biz scenarios, consider adding debug/info level logging when a duplicate is detected. This would help with troubleshooting deployment issues.
for (BizArchive bizArchive : bizArchives) { Biz biz = ArkClient.getBizFactoryService().createBiz(bizArchive); - ArkClient.getBizManagerService().registerBizIfAbsent(biz); + if (!ArkClient.getBizManagerService().registerBizIfAbsent(biz)) { + ArkLoggerFactory.getDefaultLogger().info( + "Skipping duplicate biz registration: " + biz.getIdentity()); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java(1 hunks)sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java(1 hunks)sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStageTest.java(3 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizManagerService.java(1 hunks)
🔇 Additional comments (5)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizManagerService.java (1)
160-161: LGTM on the method signature.
The method signature aligns well with the PR's objective of deduplicating module deployments and follows the interface's existing patterns.
Let's verify the implementation of this method:
✅ Verification successful
Let me gather more information about the implementation.
Implementation is thread-safe and correctly handles deduplication
The implementation is well-designed with proper thread safety using a combination of ConcurrentHashMap and synchronized block:
- Uses ConcurrentHashMap for the main bizRegistration storage
- Implements double-checked locking pattern for thread-safe registration
- Correctly uses putIfAbsent for the outer map and synchronized block for the inner map operations
- Properly validates input parameters before registration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation to ensure thread-safety and proper deduplication logic
# Look for the implementation class
rg -l "class.*BizManagerServiceImpl.*implements.*BizManagerService"
# Check the implementation of registerBizIfAbsent
ast-grep --pattern 'boolean registerBizIfAbsent(Biz $_) {
$$$
}'
Length of output: 257
Script:
#!/bin/bash
# Get the implementation details from BizManagerServiceImpl.java
rg -A 10 "boolean registerBizIfAbsent\(Biz" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java
# Check if there's any synchronization mechanism in the class
rg "synchronized|Lock|Concurrent" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java
Length of output: 2258
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStageTest.java (1)
Line range hint 19-46: LGTM! Imports are appropriate for the new test implementation.
The new imports (ArkClient, Biz, MockedStatic) are necessary to support the static mocking strategy used in the updated test.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java (1)
200-200: LGTM! Good improvement for deduplication.
The change from registerBiz to registerBizIfAbsent aligns well with the PR objective of deduplicating module deployments during static merge. This prevents potential issues with duplicate registrations while maintaining the existing functionality.
Let's verify the error handling in the new registration path:
✅ Verification successful
Error handling is properly implemented in registerBizIfAbsent
The implementation of registerBizIfAbsent in BizManagerServiceImpl has appropriate error handling:
- Input validation using
AssertUtilsto check for null and valid state - Thread-safe implementation using synchronization
- Proper state verification before registration
- Returns boolean to indicate success/failure
The change from registerBiz to registerBizIfAbsent is safe and well-implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in registerBizIfAbsent implementation
ast-grep --pattern 'registerBizIfAbsent($$$) {
$$$
throw $$$
$$$
}'
# Check for any existing error handling tests
rg -l "registerBizIfAbsent.*Exception" -t java
Length of output: 124
Script:
#!/bin/bash
# Let's first find the implementation of registerBizIfAbsent
ast-grep --pattern 'registerBizIfAbsent($$$) {
$$$
}'
# Also search for any test files related to this functionality
rg -l "registerBizIfAbsent" -t java
# Look for any exception handling patterns in the service
rg "throws|catch|Exception" -A 3 -B 3 --type java -g "*BizManagerService*"
Length of output: 4429
Script:
#!/bin/bash
# Let's look at the actual implementation of registerBizIfAbsent in BizManagerServiceImpl
rg -A 10 "registerBizIfAbsent" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java
# Also check the test coverage for this specific method
rg -A 10 "registerBizIfAbsent" sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStageTest.java
Length of output: 1390
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java (2)
200-200: LGTM! Verify error handling in BizManagerService
The change from registerBiz to registerBizIfAbsent aligns with the PR objective of deduplicating module deployments. This is a good improvement to prevent duplicate registrations during static merge deployments.
Let's verify the error handling in the new method:
✅ Verification successful
LGTM! Error handling is properly implemented
The implementation of registerBizIfAbsent in BizManagerServiceImpl has robust error handling:
- Validates non-null biz parameter using
AssertUtils.assertNotNull - Verifies biz state using
AssertUtils.isTrue - Uses thread-safe concurrent collections and proper synchronization
- Implements proper duplicate checking with double-checked locking pattern
The change is safe and well-implemented across the codebase:
- Used consistently in both
ArkContainerandHandleArchiveStage - Maintains thread safety with
ConcurrentHashMap - Returns boolean to indicate registration status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in BizManagerService implementation
# Look for error handling patterns in registerBizIfAbsent implementation
# Search for the implementation
ast-grep --pattern 'class BizManagerServiceImpl {
$$$
registerBizIfAbsent($$$) {
$$$
}
$$$
}'
# Search for exception handling
rg -A 5 "registerBizIfAbsent.*throws"
Length of output: 147
Script:
#!/bin/bash
# Let's try a broader search to locate the implementation and error handling
# Find all files containing BizManagerService
fd -e java BizManagerService
# Search for registerBizIfAbsent implementation with context
rg -A 10 "registerBizIfAbsent"
# Search for BizManagerServiceImpl class with context
rg -A 20 "class BizManagerServiceImpl"
# Look for any exception handling related to biz registration
rg -A 5 "register.*Biz.*throw"
Length of output: 11417
Line range hint 196-202: Verify consistent registration behavior across all entry points
To ensure system-wide consistency, we should verify that all biz registration entry points use the same deduplication strategy.
✅ Verification successful
Let me analyze the registration behavior across the codebase based on the search results.
Registration behavior is consistent across entry points
After analyzing the implementation and usage patterns, I can confirm that the registration behavior is consistent across all entry points:
registerBizis used in test code and internal pipeline stages for direct registrationregisterBizIfAbsentis used in production code paths (ArkContainer, HandleArchiveStage) for safe concurrent registration- The
registerBizIfAbsentimplementation properly handles thread safety with double-checked locking pattern when adding new biz entries
The new code in ArkContainer correctly uses registerBizIfAbsent, which is the appropriate method for production registration paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all biz registration calls to ensure consistent behavior
# Search for direct registration calls
rg "registerBiz\("
# Search for the new method usage
rg "registerBizIfAbsent\("
# Look for other potential registration points
ast-grep --pattern 'BizManagerService.$$$register$$$'
Length of output: 9196
Script:
#!/bin/bash
# Let's examine the implementation of both registration methods
ast-grep --pattern 'public boolean registerBiz(Biz biz) {
$$$
}'
ast-grep --pattern 'public boolean registerBizIfAbsent(Biz biz) {
$$$
}'
# Also check where these methods are primarily used
rg -B 2 -A 2 "registerBiz\(" sofa-ark-parent/core-impl/container/src/main/java/
rg -B 2 -A 2 "registerBizIfAbsent\(" sofa-ark-parent/core-impl/container/src/main/java/
Length of output: 9978
Script:
#!/bin/bash
# Let's examine the actual implementation of BizManagerServiceImpl to understand the registration logic
cat sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java
Length of output: 9957
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | ||
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | ||
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); |
There was a problem hiding this comment.
Fix incorrect test data: Biz object is null
The test is verifying registerBizIfAbsent(null), which doesn't properly test the deduplication logic. The Biz object should be created from the mocked BizArchive.
Apply this diff to fix the test:
- handleArchiveStage.processStaticBizFromClasspath(pipelineContext);
- verify(bizFactoryService, times(1)).createBiz((bizArchive));
- verify(bizManagerService, times(1)).registerBizIfAbsent(null);
+ Biz mockBiz = mock(Biz.class);
+ when(bizFactoryService.createBiz(bizArchive)).thenReturn(mockBiz);
+
+ handleArchiveStage.processStaticBizFromClasspath(pipelineContext);
+
+ verify(bizFactoryService, times(1)).createBiz(bizArchive);
+ verify(bizManagerService, times(1)).registerBizIfAbsent(mockBiz);📝 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.
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); | |
| Biz mockBiz = mock(Biz.class); | |
| when(bizFactoryService.createBiz(bizArchive)).thenReturn(mockBiz); | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| verify(bizFactoryService, times(1)).createBiz(bizArchive); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(mockBiz); |
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | ||
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | ||
| try { | ||
| BizArchive bizArchive = mock(BizArchive.class); | ||
| Manifest manifest = new Manifest(); | ||
| Attributes mainAttributes = manifest.getMainAttributes(); | ||
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | ||
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | ||
| when(bizArchive.getManifest()).thenReturn(manifest); | ||
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | ||
| when(bizManagerService.getBizRegistration()).thenReturn(map); | ||
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | ||
|
|
||
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | ||
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | ||
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); | ||
| }finally { | ||
| arkClientMockedStatic.close(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for deduplication scenarios
The test should verify the actual deduplication behavior by testing scenarios where:
- A Biz doesn't exist (should register)
- A Biz already exists (should skip registration)
Consider adding these test cases:
+ @Test
+ public void testProcessStaticBizFromClasspathWithExistingBiz() throws Exception {
+ MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class);
+ arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService);
+ try {
+ // Setup existing biz
+ ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>();
+ ConcurrentHashMap<String, Biz> versionMap = new ConcurrentHashMap<>();
+ Biz existingBiz = mock(Biz.class);
+ when(existingBiz.getBizName()).thenReturn("biz1");
+ when(existingBiz.getBizVersion()).thenReturn("0.0.1");
+ versionMap.put("0.0.1", existingBiz);
+ map.put("biz1", versionMap);
+ when(bizManagerService.getBizRegistration()).thenReturn(map);
+
+ // Setup new biz with same name and version
+ BizArchive bizArchive = mock(BizArchive.class);
+ Manifest manifest = new Manifest();
+ Attributes mainAttributes = manifest.getMainAttributes();
+ mainAttributes.putValue(ARK_BIZ_NAME, "biz1");
+ mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1");
+ when(bizArchive.getManifest()).thenReturn(manifest);
+ when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive));
+
+ Biz newBiz = mock(Biz.class);
+ when(bizFactoryService.createBiz(bizArchive)).thenReturn(newBiz);
+
+ handleArchiveStage.processStaticBizFromClasspath(pipelineContext);
+
+ // Verify biz was created but not registered due to existing registration
+ verify(bizFactoryService, times(1)).createBiz(bizArchive);
+ verify(bizManagerService, times(1)).registerBizIfAbsent(newBiz);
+ verify(bizManagerService, never()).registerBiz(any());
+ } finally {
+ arkClientMockedStatic.close();
+ }
+ }📝 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.
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | |
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | |
| try { | |
| BizArchive bizArchive = mock(BizArchive.class); | |
| Manifest manifest = new Manifest(); | |
| Attributes mainAttributes = manifest.getMainAttributes(); | |
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | |
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | |
| when(bizArchive.getManifest()).thenReturn(manifest); | |
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | |
| when(bizManagerService.getBizRegistration()).thenReturn(map); | |
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); | |
| }finally { | |
| arkClientMockedStatic.close(); | |
| } | |
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | |
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | |
| try { | |
| BizArchive bizArchive = mock(BizArchive.class); | |
| Manifest manifest = new Manifest(); | |
| Attributes mainAttributes = manifest.getMainAttributes(); | |
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | |
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | |
| when(bizArchive.getManifest()).thenReturn(manifest); | |
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | |
| when(bizManagerService.getBizRegistration()).thenReturn(map); | |
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); | |
| }finally { | |
| arkClientMockedStatic.close(); | |
| } | |
| @Test | |
| public void testProcessStaticBizFromClasspathWithExistingBiz() throws Exception { | |
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | |
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | |
| try { | |
| // Setup existing biz | |
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | |
| ConcurrentHashMap<String, Biz> versionMap = new ConcurrentHashMap<>(); | |
| Biz existingBiz = mock(Biz.class); | |
| when(existingBiz.getBizName()).thenReturn("biz1"); | |
| when(existingBiz.getBizVersion()).thenReturn("0.0.1"); | |
| versionMap.put("0.0.1", existingBiz); | |
| map.put("biz1", versionMap); | |
| when(bizManagerService.getBizRegistration()).thenReturn(map); | |
| // Setup new biz with same name and version | |
| BizArchive bizArchive = mock(BizArchive.class); | |
| Manifest manifest = new Manifest(); | |
| Attributes mainAttributes = manifest.getMainAttributes(); | |
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | |
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | |
| when(bizArchive.getManifest()).thenReturn(manifest); | |
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | |
| Biz newBiz = mock(Biz.class); | |
| when(bizFactoryService.createBiz(bizArchive)).thenReturn(newBiz); | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| // Verify biz was created but not registered due to existing registration | |
| verify(bizFactoryService, times(1)).createBiz(bizArchive); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(newBiz); | |
| verify(bizManagerService, never()).registerBiz(any()); | |
| } finally { | |
| arkClientMockedStatic.close(); | |
| } | |
| } |
| public boolean registerBizIfAbsent(Biz biz) { | ||
| AssertUtils.assertNotNull(biz, "Biz must not be null."); | ||
| AssertUtils.isTrue(biz.getBizState() == BizState.RESOLVED, "BizState must be RESOLVED."); | ||
| if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) { | ||
| bizRegistration.putIfAbsent(biz.getBizName(), new ConcurrentHashMap<>(16)); | ||
| synchronized (bizRegistration.get(biz.getBizName())) { | ||
| if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) { | ||
| ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(biz.getBizName()); | ||
| return bizCache.put(biz.getBizVersion(), biz) == null; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Thread safety concern in double-checked locking implementation.
The synchronized block locks on a ConcurrentHashMap value which could potentially be replaced by another thread, leading to synchronization issues. Consider using a more robust synchronization approach.
Here's a suggested implementation that addresses this concern:
@Override
public boolean registerBizIfAbsent(Biz biz) {
AssertUtils.assertNotNull(biz, "Biz must not be null.");
AssertUtils.isTrue(biz.getBizState() == BizState.RESOLVED, "BizState must be RESOLVED.");
- if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) {
- bizRegistration.putIfAbsent(biz.getBizName(), new ConcurrentHashMap<>(16));
- synchronized (bizRegistration.get(biz.getBizName())) {
- if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) {
- ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(biz.getBizName());
- return bizCache.put(biz.getBizVersion(), biz) == null;
- }
- }
+ String bizName = biz.getBizName();
+ String bizVersion = biz.getBizVersion();
+
+ // Create the version map if absent
+ bizRegistration.putIfAbsent(bizName, new ConcurrentHashMap<>(16));
+ ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(bizName);
+
+ // Use putIfAbsent for atomic check-then-act
+ return bizCache.putIfAbsent(bizVersion, biz) == null;
}
- return false;
}The suggested implementation:
- Eliminates the need for explicit synchronization by leveraging ConcurrentHashMap's atomic operations
- Reduces redundant getBiz calls
- Maintains thread safety while improving performance
📝 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.
| public boolean registerBizIfAbsent(Biz biz) { | |
| AssertUtils.assertNotNull(biz, "Biz must not be null."); | |
| AssertUtils.isTrue(biz.getBizState() == BizState.RESOLVED, "BizState must be RESOLVED."); | |
| if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) { | |
| bizRegistration.putIfAbsent(biz.getBizName(), new ConcurrentHashMap<>(16)); | |
| synchronized (bizRegistration.get(biz.getBizName())) { | |
| if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) { | |
| ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(biz.getBizName()); | |
| return bizCache.put(biz.getBizVersion(), biz) == null; | |
| } | |
| } | |
| } | |
| return false; | |
| } | |
| public boolean registerBizIfAbsent(Biz biz) { | |
| AssertUtils.assertNotNull(biz, "Biz must not be null."); | |
| AssertUtils.isTrue(biz.getBizState() == BizState.RESOLVED, "BizState must be RESOLVED."); | |
| String bizName = biz.getBizName(); | |
| String bizVersion = biz.getBizVersion(); | |
| // Create the version map if absent | |
| bizRegistration.putIfAbsent(bizName, new ConcurrentHashMap<>(16)); | |
| ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(bizName); | |
| // Use putIfAbsent for atomic check-then-act | |
| return bizCache.putIfAbsent(bizVersion, biz) == null; | |
| } |
💡 Codebase verification
Add concurrent test cases for registerBizIfAbsent method
The current test suite (BizManagerServiceTest.java) lacks concurrent testing for the registerBizIfAbsent method, while the codebase already has examples of concurrent testing patterns in ClassLoaderConcurrencyTest.java. Consider adding test cases that:
- Use multiple threads to register the same biz simultaneously
- Verify that only one registration succeeds
- Follow the concurrent testing pattern from
ClassLoaderConcurrencyTest.javausingCountDownLatchandThreadPoolExecutor
🔗 Analysis chain
Consider adding unit tests for concurrent scenarios.
The new method handles concurrent registration attempts, but there are no visible tests verifying this behavior.
Let's check for existing concurrent tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to BizManagerServiceImpl
fd -e java -e groovy "BizManagerServiceImpl.*Test"
# Search for concurrent testing patterns
rg -i "concurrent|thread|async" -A 5 -B 5 "BizManagerServiceImpl.*Test"
Length of output: 194
Script:
#!/bin/bash
# Let's try a broader search for test files related to BizManager
fd -e java -e groovy "BizManager.*Test"
# Search for any test files in the same directory as BizManagerServiceImpl
fd -e java -e groovy ".*Test" sofa-ark-parent/core-impl/container/src/test/
# Let's also check if there are any test files that might test concurrent behavior
rg -i "concurrent.*test|thread.*test" sofa-ark-parent/core-impl/container/src/test/
Length of output: 5464
Script:
#!/bin/bash
# Let's examine the content of BizManagerServiceTest.java to see if it has any concurrent testing
cat sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceTest.java
# Also check ClassLoaderConcurrencyTest.java as it might have patterns we can follow
cat sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/ClassLoaderConcurrencyTest.java
Length of output: 10909
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizManagerService.java (2)
160-161: Add JavaDoc documentation for the new method.For consistency with other methods in the interface, please add JavaDoc documentation that describes:
- The method's purpose
- The parameter description
- The return value meaning (true if registered, false if already exists)
+ /** + * Register Biz only if it's not already registered + * + * @param biz The business entity to register + * @return true if the biz was registered, false if it already exists + */ boolean registerBizIfAbsent(Biz biz);
160-161: LGTM! The new method enhances deduplication.The addition of
registerBizIfAbsentis a good enhancement that supports the PR's objective of deduplicating module deployments during static merge deployments. It provides an atomic way to check-and-register business entities, preventing duplicate registrations.Consider implementing this method in a thread-safe manner in the implementation class, possibly using the
putIfAbsentoperation of theConcurrentHashMapthat's already used ingetBizRegistration().sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java (1)
225-239: LGTM! Thread-safe implementation for deduplication.The implementation correctly handles concurrent registration attempts using double-checked locking pattern and maintains consistency with existing validation logic.
Consider optimizing the synchronization granularity by using a more specific lock object:
- synchronized (bizRegistration.get(biz.getBizName())) { + ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(biz.getBizName()); + synchronized (bizCache) { if (getBiz(biz.getBizName(), biz.getBizVersion()) == null) { - ConcurrentHashMap<String, Biz> bizCache = bizRegistration.get(biz.getBizName()); return bizCache.put(biz.getBizVersion(), biz) == null; } }This change:
- Reduces the number of map lookups
- Makes the synchronized block's scope more explicit
- Maintains the same thread-safety guarantees
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java (1)
199-201: Consider adding debug logging for skipped registrations.To aid in troubleshooting, consider logging when a biz registration is skipped due to an existing registration.
public void processStaticBizFromClasspath(PipelineContext pipelineContext) throws Exception { ExecutableArchive executableArchive = pipelineContext.getExecutableArchive(); List<BizArchive> bizArchives = executableArchive.getBizArchives(); for (BizArchive bizArchive : bizArchives) { Biz biz = bizFactoryService.createBiz(bizArchive); - bizManagerService.registerBizIfAbsent(biz); + if (!bizManagerService.registerBizIfAbsent(biz)) { + ArkLoggerFactory.getDefaultLogger().debug( + "Skipped registering duplicate biz: {}", biz.getIdentity()); + } } }sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java (1)
Line range hint
156-173: Consider documenting the deployment order guaranteesThe deployment lifecycle involves multiple stages (master biz → static biz → custom hooks), and the new deduplication logic makes the order of deployment more significant. Consider documenting:
- The order guarantees between different deployment stages
- How version conflicts are resolved when the same biz is registered through different paths
Would you like me to help draft the documentation for the deployment lifecycle?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java(1 hunks)sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java(1 hunks)sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStageTest.java(3 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizManagerService.java(1 hunks)
🔇 Additional comments (4)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStageTest.java (1)
Line range hint 19-46: LGTM! Import changes align with new test requirements.
The new imports support static method mocking and concurrent data structures needed for testing the deduplication logic.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java (1)
225-239: Verify integration with dependent components.
Let's confirm that all callers properly handle the return value of registerBizIfAbsent.
✅ Verification successful
Let me gather more context about error handling in the calling methods.
Integration with dependent components is properly implemented
The verification shows that registerBizIfAbsent is used correctly in all calling contexts:
- In
ArkContainer.addStaticBiz, the method is called within a proper error handling context (wrapped in a method that declaresthrows IOException) - In
HandleArchiveStage.processStaticBizFromClasspath, it's called within a method that declaresthrows Exception - The method's preconditions are enforced through
AssertUtilschecks - The return value (boolean) is appropriately used as a status indicator, not requiring explicit handling since a false return simply means the biz was already registered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of registerBizIfAbsent and verify their error handling
# Search for direct method calls
echo "Searching for direct callers of registerBizIfAbsent..."
rg -A 5 "registerBizIfAbsent\("
# Search for potential error handling patterns around the calls
echo "Checking error handling patterns..."
ast-grep --pattern 'if ($service.registerBizIfAbsent($_)) {
$$$
}'
Length of output: 4234
Script:
#!/bin/bash
# Check the implementation of methods that call registerBizIfAbsent
# Check ArkContainer.addStaticBiz implementation
echo "Checking ArkContainer.addStaticBiz implementation:"
rg -B 5 -A 10 "addStaticBiz" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java
# Check HandleArchiveStage.processStaticBizFromClasspath implementation
echo -e "\nChecking HandleArchiveStage.processStaticBizFromClasspath implementation:"
rg -B 5 -A 10 "processStaticBizFromClasspath" sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java
# Check if there's any exception handling around these calls
echo -e "\nChecking for try-catch blocks around registerBizIfAbsent calls:"
ast-grep --pattern 'try {
$$$
registerBizIfAbsent($$$)
$$$
} catch ($$$) {
$$$
}'
Length of output: 2806
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/pipeline/HandleArchiveStage.java (1)
200-200: LGTM! The change aligns with deduplication objectives.
The switch to registerBizIfAbsent effectively prevents duplicate module registrations during static merge deployments, which is the core objective of this PR.
Let's verify the thread safety and implementation of registerBizIfAbsent:
✅ Verification successful
Let me search for the implementation of registerBizIfAbsent to verify thread safety.
Let me check the implementation of registerBizIfAbsent in BizManagerServiceImpl.java to verify thread safety.
Thread-safe implementation confirmed, change is safe
The registerBizIfAbsent implementation in BizManagerServiceImpl is thread-safe:
- Uses
ConcurrentHashMapfor both levels of caching - Implements proper double-checked locking pattern with synchronized block
- Maintains consistent state checks and validations
The change from registerBiz to registerBizIfAbsent is a safe improvement that:
- Prevents duplicate registrations
- Maintains thread safety
- Preserves first-registered instance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of registerBizIfAbsent
ast-grep --pattern 'registerBizIfAbsent($$$) {
$$$
}'
# Check for other registration points to ensure consistent behavior
rg -A 5 'registerBiz\(' --type java
Length of output: 42474
Script:
#!/bin/bash
# Search for the implementation of registerBizIfAbsent
ast-grep --pattern 'registerBizIfAbsent($$$) {
$$$
}'
# Also search for any file containing this method declaration
rg 'registerBizIfAbsent' --type java
Length of output: 933
Script:
#!/bin/bash
# Get the implementation of registerBizIfAbsent
cat sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizManagerServiceImpl.java
Length of output: 9957
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/ArkContainer.java (1)
200-200: LGTM! Verify thread safety and version handling
The change from registerBiz to registerBizIfAbsent aligns with the PR objective of deduplicating module deployments. This prevents duplicate registration while maintaining first-registered-wins semantics.
Let's verify the implementation of registerBizIfAbsent and its thread safety:
✅ Verification successful
Based on the shell script results, I can now verify the thread safety and version handling in the registerBizIfAbsent implementation:
-
The implementation in
BizManagerServiceImpluses proper synchronization:- Uses
ConcurrentHashMapfor the bizRegistration map - Has synchronized block on the biz name's map for atomic check-then-act
- Performs version check via
getBizbefore registration
- Uses
-
The version handling is robust:
- Checks for existing biz with same name and version before registration
- Uses two-level concurrent map structure for name->version->biz mapping
- Maintains proper state validation (RESOLVED state check)
-
The change is used consistently:
- Only used in static biz deployment scenarios
- Other registration paths still use
registerBizfor strict control
The implementation is thread-safe and handles versioning correctly
The change from registerBiz to registerBizIfAbsent is properly implemented with thread-safe concurrent collections and synchronized blocks. The version handling ensures no duplicate registrations of the same biz version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of registerBizIfAbsent and related synchronization
# Check BizManagerService implementation for thread safety
ast-grep --pattern 'class BizManagerServiceImpl {
$$$
registerBizIfAbsent($$$) {
$$$
}
$$$
}'
# Check for potential race conditions in concurrent registrations
rg -A 5 'registerBizIfAbsent|registerBiz' --type java
# Check version handling in duplicate scenarios
ast-grep --pattern 'class BizManagerServiceImpl {
$$$
getBiz($$$) {
$$$
}
$$$
}'
Length of output: 51307
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | ||
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | ||
| try { | ||
| BizArchive bizArchive = mock(BizArchive.class); | ||
| Manifest manifest = new Manifest(); | ||
| Attributes mainAttributes = manifest.getMainAttributes(); | ||
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | ||
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | ||
| when(bizArchive.getManifest()).thenReturn(manifest); | ||
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | ||
| when(bizManagerService.getBizRegistration()).thenReturn(map); | ||
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | ||
|
|
||
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | ||
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | ||
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); | ||
| }finally { | ||
| arkClientMockedStatic.close(); | ||
| } |
There was a problem hiding this comment.
Test implementation needs improvement
Several issues need to be addressed:
- The verification
registerBizIfAbsent(null)on line 173 appears incorrect as we're creating a biz archive and factory but not verifying the actual biz object. - The test doesn't verify the deduplication behavior which is the main objective of this PR.
- Missing verification of manifest attributes usage.
Consider refactoring the test like this:
@Test
public void testProcessStaticBizFromClasspath() throws Exception {
MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class);
arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService);
try {
BizArchive bizArchive = mock(BizArchive.class);
Manifest manifest = new Manifest();
Attributes mainAttributes = manifest.getMainAttributes();
mainAttributes.putValue(ARK_BIZ_NAME, "biz1");
mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1");
when(bizArchive.getManifest()).thenReturn(manifest);
ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>();
when(bizManagerService.getBizRegistration()).thenReturn(map);
when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive));
+ // Create and configure mock Biz
+ Biz mockBiz = mock(Biz.class);
+ when(mockBiz.getBizName()).thenReturn("biz1");
+ when(mockBiz.getBizVersion()).thenReturn("0.0.1");
+ when(bizFactoryService.createBiz(bizArchive)).thenReturn(mockBiz);
handleArchiveStage.processStaticBizFromClasspath(pipelineContext);
- verify(bizFactoryService, times(1)).createBiz((bizArchive));
- verify(bizManagerService, times(1)).registerBizIfAbsent(null);
+ verify(bizFactoryService, times(1)).createBiz(bizArchive);
+ verify(bizManagerService, times(1)).registerBizIfAbsent(mockBiz);
+
+ // Test deduplication
+ handleArchiveStage.processStaticBizFromClasspath(pipelineContext);
+ // Should still be called only once total
+ verify(bizManagerService, times(1)).registerBizIfAbsent(mockBiz);
}finally {
arkClientMockedStatic.close();
}
}This refactored version:
- Properly creates and verifies a mock Biz object
- Tests the deduplication behavior by calling the method twice
- Verifies that registration happens only once
📝 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.
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | |
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | |
| try { | |
| BizArchive bizArchive = mock(BizArchive.class); | |
| Manifest manifest = new Manifest(); | |
| Attributes mainAttributes = manifest.getMainAttributes(); | |
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | |
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | |
| when(bizArchive.getManifest()).thenReturn(manifest); | |
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | |
| when(bizManagerService.getBizRegistration()).thenReturn(map); | |
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| verify(bizFactoryService, times(1)).createBiz((bizArchive)); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(null); | |
| }finally { | |
| arkClientMockedStatic.close(); | |
| } | |
| MockedStatic<ArkClient> arkClientMockedStatic = mockStatic(ArkClient.class); | |
| arkClientMockedStatic.when(ArkClient::getBizManagerService).thenReturn(bizManagerService); | |
| try { | |
| BizArchive bizArchive = mock(BizArchive.class); | |
| Manifest manifest = new Manifest(); | |
| Attributes mainAttributes = manifest.getMainAttributes(); | |
| mainAttributes.putValue(ARK_BIZ_NAME, "biz1"); | |
| mainAttributes.putValue(ARK_BIZ_VERSION, "0.0.1"); | |
| when(bizArchive.getManifest()).thenReturn(manifest); | |
| ConcurrentHashMap<String, ConcurrentHashMap<String, Biz>> map = new ConcurrentHashMap<>(); | |
| when(bizManagerService.getBizRegistration()).thenReturn(map); | |
| when(executableArchive.getBizArchives()).thenReturn(asList(bizArchive)); | |
| // Create and configure mock Biz | |
| Biz mockBiz = mock(Biz.class); | |
| when(mockBiz.getBizName()).thenReturn("biz1"); | |
| when(mockBiz.getBizVersion()).thenReturn("0.0.1"); | |
| when(bizFactoryService.createBiz(bizArchive)).thenReturn(mockBiz); | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| verify(bizFactoryService, times(1)).createBiz(bizArchive); | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(mockBiz); | |
| // Test deduplication | |
| handleArchiveStage.processStaticBizFromClasspath(pipelineContext); | |
| // Should still be called only once total | |
| verify(bizManagerService, times(1)).registerBizIfAbsent(mockBiz); | |
| }finally { | |
| arkClientMockedStatic.close(); | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1035 +/- ##
============================================
+ Coverage 78.20% 78.29% +0.08%
- Complexity 874 875 +1
============================================
Files 171 171
Lines 7056 7066 +10
Branches 1037 1041 +4
============================================
+ Hits 5518 5532 +14
+ Misses 1006 997 -9
- Partials 532 537 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
static merge deployments optimize issue 338
Summary by CodeRabbit
New Features
Bug Fixes
Tests