Feat: add batch install biz handler #231
Conversation
WalkthroughThis pull request introduces a comprehensive batch installation feature for the Arklet framework. The changes span multiple files, adding a new Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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: 11
🔭 Outside diff range comments (3)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java (1)
Line range hint
136-147: Translate comments and add file cleanupThe method contains Chinese comments and doesn't clean up temporary files after use.
private static void refreshBizInfoFromJar(Input input) throws IOException { - // 如果入参里没有jar，例如模块卸载，这里就直接返回 + // If no jar in input (e.g., module uninstallation), return directly if (StringUtils.isEmpty(input.getBizUrl())) { return; } BizFactoryService bizFactoryService = ArkClient.getBizFactoryService(); URL url = new URL(input.getBizUrl()); File bizFile = ArkClient.createBizSaveFile(input.getBizName(), input.getBizVersion()); - FileUtils.copyInputStreamToFile(url.openStream(), bizFile); - Biz biz = bizFactoryService.createBiz(bizFile); - input.setBizName(biz.getBizName()); - input.setBizVersion(biz.getBizVersion()); + try { + FileUtils.copyInputStreamToFile(url.openStream(), bizFile); + Biz biz = bizFactoryService.createBiz(bizFile); + input.setBizName(biz.getBizName()); + input.setBizVersion(biz.getBizVersion()); + } finally { + if (bizFile != null && bizFile.exists()) { + bizFile.delete(); + } + } }pom.xml (2)
Line range hint
6-6: Fix typo in maven-gpg-plugin property nameThere's a typo in the property name:
maven.gpg.pluignshould bemaven.gpg.plugin. This property is used in the release and snapshot profiles.Apply this diff to fix the typo:
- <maven.gpg.pluign>1.6</maven.gpg.pluign> + <maven.gpg.plugin>1.6</maven.gpg.plugin>Also applies to: 168-168, 171-171
Security vulnerabilities found in dependencies - Updates required
Based on the security scan results:
- jackson-databind 2.13.5 has known HIGH severity vulnerabilities. Latest version 2.13.4+ patches resource consumption vulnerabilities.
- fastjson 1.2.83 is the patched version that fixes HIGH severity unsafe deserialization vulnerabilities. The current version appears to be using variables, but if it's lower than 1.2.83, it needs updating.
- guava has MODERATE severity vulnerabilities in versions below 32.0.0-android, including a temporary directory security issue.
Action items:
- Update jackson-databind to at least 2.13.4 or preferably to the latest 2.15.x version
- Ensure fastjson version is at least 1.2.83
- Update guava to at least 32.0.0-android or latest 32.1.3-jre
🔗 Analysis chain
Line range hint
6-6: Review security vulnerabilities in dependenciesSeveral dependencies should be checked for known vulnerabilities:
- jackson-databind 2.13.5
- fastjson 1.2.83
- guava 32.1.3-jre
Also applies to: 168-171
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories for critical dependencies echo "Checking jackson-databind..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.fasterxml.jackson.core:jackson-databind") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' echo "Checking fastjson..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.alibaba:fastjson") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 2485
Script:
#!/bin/bash # Let's check the actual versions used in pom.xml echo "Checking dependency versions in pom.xml..." rg -A 3 "<dependency>" pom.xml | rg -e "<dependency>" -e "<groupId>" -e "<artifactId>" -e "<version>" # Also check for guava version specifically echo -e "\nChecking guava version..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.google.guava:guava") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 6759
🧹 Nitpick comments (11)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/BatchInstallBizHandler.java (4)
49-57: Validate Each InstallRequest Thoroughly
You have a loop enforcing basic validation on each biz, but consider additional checks (e.g., ensuring non-null bizUrl, valid version format, etc.) before constructing the request object. This can avoid runtime errors downstream.
59-75: Ensure Proper Context for Exceptions
When an exception occurs during batch installation, it’s caught and rethrown as ArkletRuntimeException. Consider appending a clear context message (e.g., "Batch installation failed") to aid debugging.
82-93: Consider Enhancing Logging for Batch Request Details
Building install requests in a loop can produce varied results if the input array is large or if any data is missing. Logging details about each constructed InstallRequest can help identify problematic inputs faster.
95-101: Ensure Matching Response Types
This method converts a model response to BatchInstallResponse, but if the internal structure changes (e.g., new fields), the manual mapping could become outdated. Consider a safer mapping strategy or a single unified response model.arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/MqttMessageHandler.java (4)
238-239: Clarify Map Key Constants
"bizList" is a string literal key. Using a static constant or enum for such keys helps avoid potential typos and eases refactoring.
242-243: Improve Baseline Playback Logging
The log statement is functional, but adding contextual data (e.g., request count, environment details, etc.) can simplify troubleshooting.
249-250: Enhance Failure Logging
Currently, the logged message mostly references the exception. Including the command content or partial stack trace in the log might be helpful in identifying where or why the failure occurred.
256-256: Additional Success Message Guidance
When a batch install is successful, consider adding more details about the success, such as the size of the batch or the final statuses of each item, improving observability.arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/common/model/BatchInstallRequest.java (1)
50-50: Consider using List instead of array for installRequestsUsing an array for
installRequestscreates a fixed-size collection that's less flexible than a List. Consider usingList<InstallRequest>instead, which would:
- Provide more flexibility for dynamic sizing
- Offer rich collection methods
- Better align with Java collections best practices
- private InstallRequest[] installRequests = new InstallRequest[0]; + private List<InstallRequest> installRequests = new ArrayList<>();arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/BuiltinCommand.java (1)
34-36: Maintain consistent formatting with other enum constantsThe formatting of the new enum constant differs from others. Consider aligning it with the existing style.
- BATCH_INSTALL_BIZ("batchInstallBiz", - "install one ark biz"), - + BATCH_INSTALL_BIZ("batchInstallBiz", + "install multiple ark biz modules in batch"),arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java (1)
Line range hint
64-75: Add memory leak monitoringWhile tracking metaspace usage is good, consider adding alerts for potential memory leaks.
MemoryPoolMXBean metaSpaceMXBean = ResourceUtils.getMetaSpaceMXBean(); long startSpace = metaSpaceMXBean.getUsage().getUsed(); +long threshold = 10 * 1024 * 1024; // 10MB threshold try { InstallBizClientResponse installBizClientResponse = convertClientResponse( getOperationService().install(convertInstallRequest(input))); - installBizClientResponse - .setElapsedSpace(metaSpaceMXBean.getUsage().getUsed() - startSpace); + long spaceUsed = metaSpaceMXBean.getUsage().getUsed() - startSpace; + installBizClientResponse.setElapsedSpace(spaceUsed); + if (spaceUsed > threshold) { + LOGGER.warn("High memory usage detected during installation: {}MB", spaceUsed / (1024 * 1024)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/BuiltinCommand.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/BatchInstallBizHandler.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java(4 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/common/model/BatchInstallRequest.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/UnifiedOperationServiceImpl.java(6 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInDirAbsolutePathStrategy.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInRequestStrategy.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallStrategy.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/util/ResourceUtils.java(1 hunks)arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/MqttMessageHandler.java(2 hunks)pom.xml(1 hunks)
🔇 Additional comments (5)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInRequestStrategy.java (1)
53-65: Validate Remote Resource Availability
When downloading the file from the provided URL, consider handling possible network or file corruption scenarios more explicitly. Although @SneakyThrows is used, it might be beneficial to incorporate a fallback or retry strategy if network reliability is a concern.
arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/MqttMessageHandler.java (2)
29-29: Remove Unused Import If Not Required
Ensure that BatchInstallResponse is actually used in this class. If not, removing the import statement will improve maintainability.
257-258: Add Output Data to Failure Logs
On failure, logging the entire Output object is helpful, but also ensure sensitive data is either masked or omitted if it may contain PII or credentials.
pom.xml (2)
12-12: Version bump aligns with new feature development
The version change to 1.4.1-SNAPSHOT is appropriate for the new batch install feature development. This change will propagate to all modules using ${revision}.
Line range hint 6-7: Verify version change impact
The version change affects the artifact ID koupleless-runtime and all its modules. Please ensure all dependent projects are aware of this change.
Also applies to: 12-12
✅ Verification successful
Version change is properly managed across the project
The version change is safely managed through Maven's ${revision} property, which is defined in the root pom.xml as 1.4.1-SNAPSHOT. All modules consistently use this property for versioning, ensuring synchronized version updates across the entire project. No hardcoded versions or inconsistent version references were found for koupleless-runtime.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any hardcoded version references that might need updating
rg -g '!pom.xml' '1\.4\.0'
# Check for any direct dependencies on koupleless-runtime in other projects
rg -g 'pom.xml' 'com\.alipay\.sofa\.koupleless.*1\.4\.0'
Length of output: 85
Script:
#!/bin/bash
# Check for any references to koupleless-runtime in pom.xml files
rg -g 'pom.xml' 'koupleless-runtime'
# Check for any references to the artifact group ID
rg -g 'pom.xml' 'com\.alipay\.sofa\.koupleless'
# List all pom.xml files to understand project structure
fd 'pom.xml$'
Length of output: 6449
Script:
#!/bin/bash
# Check for version property definition in the root pom.xml
rg -g 'pom.xml' '<revision>' -A 1
# Check if modules are using the ${revision} property
rg -g 'pom.xml' '<version>\$\{revision\}</version>'
# Check for any explicit version numbers in the dependencies
rg -g 'pom.xml' '<artifactId>koupleless-runtime</artifactId>(\s*<version>[^$]|(\s*<version>\$\{(?!revision)[^}]+\}))'
Length of output: 3795
| import com.alipay.sofa.ark.api.ArkClient; | ||
| import com.alipay.sofa.ark.common.util.FileUtils; | ||
| import com.alipay.sofa.ark.spi.service.PriorityOrdered; | ||
| import com.alipay.sofa.koupleless.arklet.core.common.model.BatchInstallRequest; | ||
| import com.alipay.sofa.koupleless.arklet.core.common.model.InstallRequest; | ||
| import com.alipay.sofa.koupleless.arklet.core.ops.BatchInstallHelper; | ||
| import lombok.SneakyThrows; | ||
|
|
||
| import java.io.File; | ||
| import java.net.URL; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.TreeMap; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Granular Exception Handling Instead of Throwing Throwable
The method signature declares "throws Throwable" and can hide specific error types. To improve clarity and maintainability, consider throwing narrower exceptions (e.g., IOException, RuntimeException) so callers can handle them precisely.
| Output<?> output = null; | ||
| try { | ||
| output = commandService.process(BuiltinCommand.BATCH_INSTALL_BIZ.getId(), cmdContent); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
Check for Null or Empty Return
If the commandService.process call fails to build a valid output object (e.g., returns null), subsequent code might fail. Consider a null check or fallback logic.
| public interface BatchInstallStrategy { | ||
| Map<Integer, List<InstallRequest>> convertToInstallInput(BatchInstallRequest request) throws Throwable; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limit the Scope of Thrown Exceptions
Like in the other classes, consider refining "throws Throwable" to more specific checked or unchecked exceptions for clearer API contracts and improved debuggability.
| /** | ||
| * 模块批量发布请求。 | ||
| */ | ||
| private InstallRequest[] installRequests = new InstallRequest[0]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints for installRequests
The field lacks validation annotations. Consider adding:
@NotNullto prevent null assignments@Size(min = 1)to ensure at least one request is provided
/**
* 模块批量发布请求。
*/
+ @NotNull
+ @Size(min = 1, message = "At least one install request must be provided")
private InstallRequest[] installRequests = new InstallRequest[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * 模块批量发布请求。 | |
| */ | |
| private InstallRequest[] installRequests = new InstallRequest[0]; | |
| /** | |
| * 模块批量发布请求。 | |
| */ | |
| @NotNull | |
| @Size(min = 1, message = "At least one install request must be provided") | |
| private InstallRequest[] installRequests = new InstallRequest[0]; |
| for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans) | ||
| if (memoryPoolMXBean.getName().equals("Metaspace")) | ||
| metaSpaceMXBean = memoryPoolMXBean; |
There was a problem hiding this comment.
Add curly braces to improve code safety
The for loop without curly braces is prone to errors during maintenance. Always use curly braces for control structures.
- for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans)
- if (memoryPoolMXBean.getName().equals("Metaspace"))
- metaSpaceMXBean = memoryPoolMXBean;
+ for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans) {
+ if (memoryPoolMXBean.getName().equals("Metaspace")) {
+ metaSpaceMXBean = memoryPoolMXBean;
+ }
+ }📝 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.
| for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans) | |
| if (memoryPoolMXBean.getName().equals("Metaspace")) | |
| metaSpaceMXBean = memoryPoolMXBean; | |
| for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans) { | |
| if (memoryPoolMXBean.getName().equals("Metaspace")) { | |
| metaSpaceMXBean = memoryPoolMXBean; | |
| } | |
| } |
| BATCH_INSTALL_BIZ("batchInstallBiz", | ||
| "install one ark biz"), |
There was a problem hiding this comment.
Fix incorrect description for BATCH_INSTALL_BIZ
The description "install one ark biz" is incorrect for a batch operation. It should reflect that this command handles multiple installations.
- BATCH_INSTALL_BIZ("batchInstallBiz",
- "install one ark biz"),
+ BATCH_INSTALL_BIZ("batchInstallBiz",
+ "install multiple ark biz modules in batch"),📝 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.
| BATCH_INSTALL_BIZ("batchInstallBiz", | |
| "install one ark biz"), | |
| BATCH_INSTALL_BIZ("batchInstallBiz", | |
| "install multiple ark biz modules in batch"), |
| private InstallRequest buildInstallRequest(String bizAbsolutePath) { | ||
| String bizUrl = OSUtils.getLocalFileProtocolPrefix() + bizAbsolutePath; | ||
| Map<String, Object> mainAttributes = batchInstallHelper.getMainAttributes(bizAbsolutePath); | ||
| String bizName = (String) mainAttributes.get(Constants.ARK_BIZ_NAME); | ||
| String bizVersion = (String) mainAttributes.get(Constants.ARK_BIZ_VERSION); | ||
|
|
||
| return InstallRequest.builder().bizUrl(bizUrl).bizName(bizName).bizVersion(bizVersion) | ||
| .installStrategy(installStrategy).build(); | ||
| } |
There was a problem hiding this comment.
Add validation for required attributes
The buildInstallRequest method should validate that required attributes (bizName, bizVersion) are present in the mainAttributes map.
private InstallRequest buildInstallRequest(String bizAbsolutePath) {
String bizUrl = OSUtils.getLocalFileProtocolPrefix() + bizAbsolutePath;
Map<String, Object> mainAttributes = batchInstallHelper.getMainAttributes(bizAbsolutePath);
+ if (mainAttributes == null) {
+ throw new IllegalStateException("Failed to read manifest attributes from: " + bizAbsolutePath);
+ }
String bizName = (String) mainAttributes.get(Constants.ARK_BIZ_NAME);
String bizVersion = (String) mainAttributes.get(Constants.ARK_BIZ_VERSION);
+ if (bizName == null || bizVersion == null) {
+ throw new IllegalStateException("Missing required attributes (name/version) in: " + bizAbsolutePath);
+ }
return InstallRequest.builder().bizUrl(bizUrl).bizName(bizName).bizVersion(bizVersion)
.installStrategy(installStrategy).build();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private InstallRequest buildInstallRequest(String bizAbsolutePath) { | |
| String bizUrl = OSUtils.getLocalFileProtocolPrefix() + bizAbsolutePath; | |
| Map<String, Object> mainAttributes = batchInstallHelper.getMainAttributes(bizAbsolutePath); | |
| String bizName = (String) mainAttributes.get(Constants.ARK_BIZ_NAME); | |
| String bizVersion = (String) mainAttributes.get(Constants.ARK_BIZ_VERSION); | |
| return InstallRequest.builder().bizUrl(bizUrl).bizName(bizName).bizVersion(bizVersion) | |
| .installStrategy(installStrategy).build(); | |
| } | |
| private InstallRequest buildInstallRequest(String bizAbsolutePath) { | |
| String bizUrl = OSUtils.getLocalFileProtocolPrefix() + bizAbsolutePath; | |
| Map<String, Object> mainAttributes = batchInstallHelper.getMainAttributes(bizAbsolutePath); | |
| if (mainAttributes == null) { | |
| throw new IllegalStateException("Failed to read manifest attributes from: " + bizAbsolutePath); | |
| } | |
| String bizName = (String) mainAttributes.get(Constants.ARK_BIZ_NAME); | |
| String bizVersion = (String) mainAttributes.get(Constants.ARK_BIZ_VERSION); | |
| if (bizName == null || bizVersion == null) { | |
| throw new IllegalStateException("Missing required attributes (name/version) in: " + bizAbsolutePath); | |
| } | |
| return InstallRequest.builder().bizUrl(bizUrl).bizName(bizName).bizVersion(bizVersion) | |
| .installStrategy(installStrategy).build(); | |
| } |
| @Override | ||
| public Map<Integer, List<InstallRequest>> convertToInstallInput(BatchInstallRequest request) throws Throwable { | ||
| Map<Integer, List<InstallRequest>> result = new TreeMap<>(); | ||
|
|
||
| Map<Integer, List<String>> bizUrls = batchInstallHelper | ||
| .getBizUrlsFromLocalFileSystem(request.getBizDirAbsolutePath()); | ||
| for (Map.Entry<Integer, List<String>> entry : bizUrls.entrySet()) { | ||
| Integer order = entry.getKey(); | ||
| for (String bizUrl : entry.getValue()) { | ||
| result.putIfAbsent(order, new ArrayList<>()); | ||
| result.get(order).add(buildInstallRequest(bizUrl)); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Add error handling for file system operations
The convertToInstallInput method should handle potential file system errors gracefully. Consider wrapping the getBizUrlsFromLocalFileSystem call in a try-catch block and providing meaningful error messages.
@Override
public Map<Integer, List<InstallRequest>> convertToInstallInput(BatchInstallRequest request) throws Throwable {
Map<Integer, List<InstallRequest>> result = new TreeMap<>();
- Map<Integer, List<String>> bizUrls = batchInstallHelper
- .getBizUrlsFromLocalFileSystem(request.getBizDirAbsolutePath());
+ Map<Integer, List<String>> bizUrls;
+ try {
+ bizUrls = batchInstallHelper.getBizUrlsFromLocalFileSystem(request.getBizDirAbsolutePath());
+ } catch (IOException e) {
+ throw new IllegalStateException("Failed to read business files from directory: " +
+ request.getBizDirAbsolutePath(), e);
+ }📝 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.
| @Override | |
| public Map<Integer, List<InstallRequest>> convertToInstallInput(BatchInstallRequest request) throws Throwable { | |
| Map<Integer, List<InstallRequest>> result = new TreeMap<>(); | |
| Map<Integer, List<String>> bizUrls = batchInstallHelper | |
| .getBizUrlsFromLocalFileSystem(request.getBizDirAbsolutePath()); | |
| for (Map.Entry<Integer, List<String>> entry : bizUrls.entrySet()) { | |
| Integer order = entry.getKey(); | |
| for (String bizUrl : entry.getValue()) { | |
| result.putIfAbsent(order, new ArrayList<>()); | |
| result.get(order).add(buildInstallRequest(bizUrl)); | |
| } | |
| } | |
| return result; | |
| } | |
| @Override | |
| public Map<Integer, List<InstallRequest>> convertToInstallInput(BatchInstallRequest request) throws Throwable { | |
| Map<Integer, List<InstallRequest>> result = new TreeMap<>(); | |
| Map<Integer, List<String>> bizUrls; | |
| try { | |
| bizUrls = batchInstallHelper.getBizUrlsFromLocalFileSystem(request.getBizDirAbsolutePath()); | |
| } catch (IOException e) { | |
| throw new IllegalStateException("Failed to read business files from directory: " + | |
| request.getBizDirAbsolutePath(), e); | |
| } | |
| for (Map.Entry<Integer, List<String>> entry : bizUrls.entrySet()) { | |
| Integer order = entry.getKey(); | |
| for (String bizUrl : entry.getValue()) { | |
| result.putIfAbsent(order, new ArrayList<>()); | |
| result.get(order).add(buildInstallRequest(bizUrl)); | |
| } | |
| } | |
| return result; | |
| } |
| private BatchInstallStrategy getBatchInstallStrategy(BatchInstallRequest request) { | ||
| if (StringUtil.isNotEmpty(request.getBizDirAbsolutePath())) { | ||
| return batchInstallBizInRequestStrategy; | ||
| } | ||
| return batchInstallBizInRequestStrategy; | ||
| } |
There was a problem hiding this comment.
Fix incorrect strategy selection logic
The getBatchInstallStrategy method always returns batchInstallBizInRequestStrategy regardless of the condition. This appears to be a bug.
private BatchInstallStrategy getBatchInstallStrategy(BatchInstallRequest request) {
if (StringUtil.isNotEmpty(request.getBizDirAbsolutePath())) {
- return batchInstallBizInRequestStrategy;
+ return batchInstallBizInDirAbsolutePathStrategy;
}
return batchInstallBizInRequestStrategy;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private BatchInstallStrategy getBatchInstallStrategy(BatchInstallRequest request) { | |
| if (StringUtil.isNotEmpty(request.getBizDirAbsolutePath())) { | |
| return batchInstallBizInRequestStrategy; | |
| } | |
| return batchInstallBizInRequestStrategy; | |
| } | |
| private BatchInstallStrategy getBatchInstallStrategy(BatchInstallRequest request) { | |
| if (StringUtil.isNotEmpty(request.getBizDirAbsolutePath())) { | |
| return batchInstallBizInDirAbsolutePathStrategy; | |
| } | |
| return batchInstallBizInRequestStrategy; | |
| } |
| ThreadPoolExecutor executorService = ExecutorServiceManager.getArkBizOpsExecutor(); | ||
| Map<String, ClientResponse> bizUrlToInstallResult = new HashMap<>(); | ||
| boolean hasFailed = false; | ||
| for (Map.Entry<Integer, List<String>> entry : bizUrls.entrySet()) { | ||
| List<String> bizUrlsInSameOrder = entry.getValue(); | ||
| for (Entry<Integer, List<InstallRequest>> entry : installRequestsWithOrder.entrySet()) { | ||
| List<InstallRequest> bizRequestInSameOrder = entry.getValue(); | ||
| List<CompletableFuture<ClientResponse>> futures = new ArrayList<>(); | ||
| for (String bizUrl : bizUrlsInSameOrder) { | ||
| futures.add(CompletableFuture.supplyAsync( | ||
| () -> safeBatchInstall(bizUrl, request.getInstallStrategy()), executorService)); | ||
| for (InstallRequest bizRequest : bizRequestInSameOrder) { | ||
| futures.add(CompletableFuture.supplyAsync(() -> safeBatchInstall(bizRequest), | ||
| executorService)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add concurrent execution limits for batch installations
The current implementation might overwhelm the system if there are too many concurrent installations. Consider adding a limit to the number of concurrent installations.
ThreadPoolExecutor executorService = ExecutorServiceManager.getArkBizOpsExecutor();
+int maxConcurrentInstalls = Math.min(bizRequestInSameOrder.size(), 5); // Adjust the limit as needed
+List<List<InstallRequest>> batches = Lists.partition(bizRequestInSameOrder, maxConcurrentInstalls);
Map<String, ClientResponse> bizUrlToInstallResult = new HashMap<>();
boolean hasFailed = false;
-for (InstallRequest bizRequest : bizRequestInSameOrder) {
- futures.add(CompletableFuture.supplyAsync(() -> safeBatchInstall(bizRequest),
- executorService));
+for (List<InstallRequest> batch : batches) {
+ for (InstallRequest bizRequest : batch) {
+ futures.add(CompletableFuture.supplyAsync(() -> safeBatchInstall(bizRequest),
+ executorService));
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| Output<?> output = null; | ||
| try { | ||
| output = commandService.process(BuiltinCommand.BATCH_INSTALL_BIZ.getId(), cmdContent); |
There was a problem hiding this comment.
看下输出是否和 moduleController 对得上
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/handler/PlaybackBaselineHandler.java (4)
43-43: Consider using CONSTANT_CASE for constants.The field
Mqtt_CMDis effectively a constant, so you might rename it toMQTT_CMDto comply with common Java naming conventions.
70-79: Add logging or specialized handling for publish failures.In
pubThrowableMsg, failures frommqttClient.publishare rethrown asArkletRuntimeException. Consider adding logs or fallback handling. This can help in diagnosing repeated publish errors and avoid silent failures in production.
95-100: Handle potential malformed MQTT messages.
JSONObject.parseObject(msg.toString(), List.class)will throw a runtime exception ifmsg.toString()is not valid JSON or not an array. Consider adding validation or a fallback scenario to gracefully handle malformed or unexpected payloads.
108-113: De-duplicate success and failure response-building code if possible.
successOfResponseandfailOfResponseproduce similar response maps. Extracting common logic into a private helper method can reduce duplication and simplify maintenance.arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/MqttMessageHandler.java (2)
81-84: Enhance error handling in handler initializationThe initialization could fail silently if handler construction throws an exception. Consider wrapping it in a try-catch block and throwing an ArkletInitException to maintain consistent error handling.
private void initMsgHandlers() { + try { msgHandlers.put(PlaybackBaselineHandler.Mqtt_CMD, new PlaybackBaselineHandler(commandService, baseMetadataHook, mqttClient, baseEnv)); + } catch (Exception e) { + throw new ArkletInitException("Failed to initialize message handlers", e); + } }
Line range hint
57-202: Consider enhancing the command handling architectureWhile the current implementation is solid, consider these architectural improvements:
- Create an enum or constants class for all MQTT commands to avoid string literals
- Consider using a command registry pattern for dynamic handler registration
- Add metrics/logging for command execution to aid in monitoring and debugging
This would make the system more maintainable and easier to extend with new commands.
arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/handler/MsgHandler.java (3)
28-31: Enhance class documentation.While there is already a doc comment, consider adding a brief usage example and clarifying the responsibilities or typical lifecycle of this abstract handler to help other developers quickly integrate or extend it.
40-58: Add robust exception and success handling.
- The
handlemethod rethrows exceptions asArkletRuntimeException, which may obscure the original error context. Consider including the original cause message for easier troubleshooting.- The informational logging messages are beneficial, but you may want to unify the grammar in log statements (e.g., “finished handling MQTT cmd”).
} catch (Throwable e) { ArkletLoggerFactory.getDefaultLogger().error(String.format( "Failed to handle mqtt cmd %s with content: %s", getMqttCMD(), e.getMessage())); pubThrowableMsg(e); - throw new ArkletRuntimeException(e); + throw new ArkletRuntimeException("Error in MsgHandler.handle: " + e.getMessage(), e); }
60-69: Consider adding Javadoc for abstract methods.Providing a brief explanation of each abstract method's purpose and expected parameter/return data helps implementers correctly override them, reducing guesswork.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/CommandServiceImpl.java(2 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/BatchInstallBizHandler.java(1 hunks)arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/MqttMessageHandler.java(4 hunks)arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/handler/MsgHandler.java(1 hunks)arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/handler/PlaybackBaselineHandler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/BatchInstallBizHandler.java
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-test-for-jdk8-in-windows
- GitHub Check: unit-test-for-jdk8-in-linux
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (6)
arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/handler/PlaybackBaselineHandler.java (2)
65-67: Validate alignment of command name with domain logic.The
getArkletCommand()method returnsBuiltinCommand.BATCH_INSTALL_BIZ, while the MQTT command name is"baseline". Verify whether these names reflect the same domain concern (baseline vs. batch install) to avoid confusion.
90-91: Ensure topic patterns remain consistent across deployments.
getPubTopic()constructs the topic askoupleless_%s/%s/base/batchInstallBizResponse. Confirm that this topic is consistent with the rest of the system’s naming scheme, avoiding potential conflicts across different commands or teams.arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/MqttMessageHandler.java (1)
64-64: LGTM: Well-structured handler pattern implementationThe final Map for message handlers promotes immutability and the handler pattern enables better separation of concerns.
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/CommandServiceImpl.java (2)
29-29: No concerns with the import.The added import for
BatchInstallBizHandleris straightforward and seems consistent with the new feature requirements.
102-102: Ensure tests and documentation for the new batch install command.Registering the new
BatchInstallBizHandlerin the built-in commands is correct. However, please verify that corresponding integration tests and documentation have been updated to reflect the new batch install functionality and that possible concurrency edge cases (e.g., conflicts with other commands) are covered.arklet-tunnel-mqtt/src/main/java/com/alipay/sofa/koupleless/arklet/tunnel/mqtt/paho/handler/MsgHandler.java (1)
1-16: License header check.The Apache 2.0 license header appears correct, and no further changes are required here.
| private Map<String, Object> failOfResponse(Throwable e) { | ||
| ClientResponse data = new ClientResponse(); | ||
| data.setMessage(ExceptionUtils.getStackTraceAsString(e)); | ||
| data.setCode(ResponseCode.FAILED); | ||
| Output<?> output = Output.ofFailed(data, e.getMessage()); | ||
|
|
||
| Map<String, Object> bizOperationResponse = new HashMap<>(); | ||
| bizOperationResponse.put(Constants.COMMAND, getArkletCommand().getId()); | ||
| bizOperationResponse.put(Constants.COMMAND_RESPONSE, output); | ||
| return bizOperationResponse; | ||
| } |
There was a problem hiding this comment.
Prevent sensitive data from leaking in stack traces.
failOfResponse(Throwable e) includes the full stack trace in the MQTT-published message, potentially exposing internal details. Consider masking or truncating sensitive information (e.g. credentials, internal class paths) before publishing to external channels.
| private final MqttClient mqttClient; | ||
| private String baseEnv; | ||
| private final AtomicBoolean run = new AtomicBoolean(false); | ||
| public static boolean baselineQueryComplete = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making baselineQueryComplete thread-safe
The public static boolean field could lead to race conditions in a multi-threaded environment. Consider using AtomicBoolean instead, similar to the existing run field.
- public static boolean baselineQueryComplete = false;
+ private static final AtomicBoolean baselineQueryComplete = new AtomicBoolean(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static boolean baselineQueryComplete = false; | |
| private static final AtomicBoolean baselineQueryComplete = new AtomicBoolean(false); |
| } else if (PlaybackBaselineHandler.Mqtt_CMD.equals(cmd)) { | ||
| msgHandlers.get(PlaybackBaselineHandler.Mqtt_CMD).handle(msg); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for message handler and use constants
The handler retrieval lacks a null check, which could lead to NullPointerException if the handler isn't registered. Also, consider using a constant for the command string comparison.
- } else if (PlaybackBaselineHandler.Mqtt_CMD.equals(cmd)) {
- msgHandlers.get(PlaybackBaselineHandler.Mqtt_CMD).handle(msg);
+ } else if (PlaybackBaselineHandler.Mqtt_CMD.equals(cmd)) {
+ MsgHandler handler = msgHandlers.get(PlaybackBaselineHandler.Mqtt_CMD);
+ if (handler == null) {
+ throw new ArkletRuntimeException("No handler registered for command: " + cmd);
+ }
+ handler.handle(msg);📝 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.
| } else if (PlaybackBaselineHandler.Mqtt_CMD.equals(cmd)) { | |
| msgHandlers.get(PlaybackBaselineHandler.Mqtt_CMD).handle(msg); | |
| } else if (PlaybackBaselineHandler.Mqtt_CMD.equals(cmd)) { | |
| MsgHandler handler = msgHandlers.get(PlaybackBaselineHandler.Mqtt_CMD); | |
| if (handler == null) { | |
| throw new ArkletRuntimeException("No handler registered for command: " + cmd); | |
| } | |
| handler.handle(msg); |
| public abstract class MsgHandler { | ||
|
|
||
| protected final CommandService commandService; | ||
|
|
||
| public MsgHandler(CommandService commandService) { | ||
| this.commandService = commandService; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add null validation for commandService using existing utility.
public MsgHandler(CommandService commandService) {
+ AssertUtils.assertNotNull(commandService, "CommandService cannot be null");
this.commandService = commandService;
}The codebase already has AssertUtils for null validation. Using it maintains consistency with the existing patterns.
🔗 Analysis chain
Validate the nullability of commandService.
Consider validating or annotating the constructor parameter (commandService) to ensure it's non-null. Doing so helps prevent accidental NullPointerException when working with the handler.
public MsgHandler(CommandService commandService) {
+ if (commandService == null) {
+ throw new IllegalArgumentException("CommandService cannot be null");
+ }
this.commandService = commandService;
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find classes extending MsgHandler
ast-grep --pattern 'class $_ extends MsgHandler'
# Look for existing null checks and similar validation patterns
rg "throw.*IllegalArgumentException.*null" -A 2
# Search for null-safety annotations
rg "@NonNull|@Nonnull|@NotNull" --type java
Length of output: 1925
Script:
#!/bin/bash
# Search for handler implementations in MQTT package
fd . arklet-tunnel-mqtt -t f -e java -x cat {}
# Look for AssertUtils usage
rg "AssertUtils" -A 2
# Search for MQTT-specific classes using commandService
rg "CommandService" -A 2
Length of output: 69162
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/BatchInstallHelper.java (1)
Priority parsing needs exception handling in multiple locations
The verification confirms that priority parsing is vulnerable to NumberFormatException in two locations:
BatchInstallHelper.javaBatchInstallBizInRequestStrategy.javaBoth should follow the established error handling pattern found in the codebase:
try { order = Integer.valueOf(attributes.getOrDefault("priority", PriorityOrdered.DEFAULT_PRECEDENCE).toString()); } catch (NumberFormatException e) { ArkletLoggerFactory.getDefaultLogger().warn("Invalid priority value in JAR manifest. Using default precedence.", e); order = PriorityOrdered.DEFAULT_PRECEDENCE; }🔗 Analysis chain
Line range hint
50-75: Guard against invalid priority values.When converting the
priorityattribute to an integer, a malformed or non-numeric value in the JAR's manifest could trigger aNumberFormatException. Consider a safer parsing strategy or fallback mechanism to avoid potential runtime errors.You can, for instance, explicitly handle the parsing as follows:
Integer parsedOrder; try { parsedOrder = Integer.valueOf( attributes.getOrDefault("priority", PriorityOrdered.DEFAULT_PRECEDENCE).toString() ); } catch (NumberFormatException e) { ArkletLoggerFactory.getDefaultLogger().warn("Invalid priority. Using default precedence."); parsedOrder = PriorityOrdered.DEFAULT_PRECEDENCE; }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for existing NumberFormatException handling rg -A 2 "NumberFormatException" # Search for test cases related to BatchInstallHelper fd "BatchInstallHelper.*Test.*\.java|Test.*BatchInstallHelper.*\.java" # Look for other instances of priority parsing from manifest rg -A 2 "getOrDefault.*priority.*DEFAULT_PRECEDENCE" # Search for manifest attribute parsing patterns ast-grep --pattern 'Integer.valueOf($$$)'Length of output: 3144
🧹 Nitpick comments (7)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/BatchInstallHelper.java (1)
92-92: Well-handled manifest checking with@SneakyThrows.Ensuring the manifest is present before processing attributes prevents null-pointer issues. However, using
@SneakyThrowsfor library or framework code may obscure debugging or custom error handling for callers. Consider explicitly declaring a checked exception if the calling flow needs to handle such errors more gracefully.arklet-core/src/test/java/com/alipay/sofa/koupleless/arklet/core/component/UnifiedOperationServiceImplTests.java (2)
28-28: Remove unused import if it's not required
This import doesn't seem to be utilized in the current test code.
57-58: Consider avoiding partial mocking if not required
Using a Mockito spy is typically appropriate when partial mocking is necessary. Evaluate whether a plain instance or a mock suffices.arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInDirAbsolutePathStrategy.java (1)
39-53: Add error handling for file system operations
WrappingBatchInstallHelper.getBizUrlsFromLocalFileSystem(...)with a try-catch can guard against unexpected IO or path errors and allow for meaningful error messages.arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/UnifiedOperationServiceImpl.java (3)
55-57: Consider injecting strategies
To facilitate future enhancements or testing, inject these strategy instances rather than instantiating them directly.
109-109: Evaluate concurrency constraints
If the batch is large, unbounded asynchronous calls may degrade performance. Consider a mechanism to limit or manage concurrency.
142-147: Plan for future expansions in strategy selection
If more strategies are added, you might benefit from a factory pattern or clearer naming for this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
arklet-core/pom.xml(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/BatchInstallHelper.java(2 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/UnifiedOperationServiceImpl.java(6 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInDirAbsolutePathStrategy.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInRequestStrategy.java(1 hunks)arklet-core/src/test/java/com/alipay/sofa/koupleless/arklet/core/component/UnifiedOperationServiceImplTests.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/strategy/BatchInstallBizInRequestStrategy.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit-test-for-dubbo-samples
- GitHub Check: unit-test-for-jdk8-in-windows
- GitHub Check: unit-test-for-sofaboot-samples
- GitHub Check: unit-test-for-jdk8-in-linux
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: unit-test-for-springboot-samples
🔇 Additional comments (8)
arklet-core/pom.xml (1)
19-19: Verify the sofa.ark.version property definition.The addition of version management using
${sofa.ark.version}property is good practice. However, we should ensure this property is properly defined in the parent POM.Run the following script to verify the property definition:
✅ Verification successful
Property ${sofa.ark.version} is properly defined
The property is correctly defined as 2.2.16 in the root pom.xml and is consistently used across multiple modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if sofa.ark.version property is defined in parent POM files # Check immediate parent pom.xml echo "Checking parent pom.xml for sofa.ark.version property:" rg -A 5 "sofa\.ark\.version" ../pom.xml # Check for property definition in any pom.xml file echo -e "\nChecking all pom.xml files for sofa.ark.version property:" fd -g "pom.xml" | xargs rg "sofa\.ark\.version"Length of output: 1698
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/BatchInstallHelper.java (1)
50-50: Kudos on converting the method to a static context.This change allows simpler invocation without needing an instance of
BatchInstallHelper. The approach to build a map of .jar files by priority is clear and aligns with the batch install flow.arklet-core/src/test/java/com/alipay/sofa/koupleless/arklet/core/component/UnifiedOperationServiceImplTests.java (1)
162-187: Improve verification of mocked static methods
You might want to verify howBatchInstallHelperis called (e.g., with which arguments). This aids in confirming correct usage, ensuring no unexpected calls are made.arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/ops/UnifiedOperationServiceImpl.java (5)
26-27: Imports for strategy classes look good
Having distinct strategy classes is a neat approach that keeps the installation logic modular.
84-84: Check request parameters before installing
It’s generally good practice to validatebizUrl,bizName, andbizVersionto catch null or invalid inputs early.
105-107: Ensure the correct strategy is selected
IfBatchInstallRequestincludes bothbizDirAbsolutePathand a direct list of biz requests, confirm which strategy should prevail to avoid inconsistent states.
112-113: Clarify handling of multiple install orders
If there are duplicate integer keys or out-of-order entries, ensure they’re processed predictably.
125-126: Maintain consistent mapping from requests to responses
Verify that the index-based retrieval ofInstallRequestremains valid if the list order changes.
| private InstallRequest buildInstallRequest(String bizAbsolutePath) { | ||
| String bizUrl = OSUtils.getLocalFileProtocolPrefix() + bizAbsolutePath; | ||
| Map<String, Object> mainAttributes = BatchInstallHelper.getMainAttributes(bizAbsolutePath); | ||
| String bizName = (String) mainAttributes.get(Constants.ARK_BIZ_NAME); | ||
| String bizVersion = (String) mainAttributes.get(Constants.ARK_BIZ_VERSION); | ||
|
|
||
| return InstallRequest.builder().bizUrl(bizUrl).bizName(bizName).bizVersion(bizVersion) | ||
| .installStrategy(installStrategy).build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate manifest attributes for safer installation
Ensure bizName and bizVersion are non-null. If they’re missing, consider throwing an exception or returning an error.
| for (InstallRequest bizRequest : bizRequestInSameOrder) { | ||
| futures.add(CompletableFuture.supplyAsync(() -> safeBatchInstall(bizRequest), | ||
| executorService)); |
There was a problem hiding this comment.
Safeguard exceptions in async tasks
Asynchronous exceptions might be swallowed. Verify sufficient logging or reporting is done in safeBatchInstall(...).
* feat: add batch install handler in arklet * update version to 1.4.1-SNAPSHOT (cherry picked from commit 029ce76)
2 participants
新增批量安装模块的 handler，基线回放的功能应该通过实现
com.alipay.sofa.ark.spi.service.biz.AddBizToStaticDeployHook接口来实现fix: koupleless/koupleless#361
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores