Repository navigation
[CSTACKEX-339] ASUP Upstream review comments - #117
Conversation
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.82% |
| Branch coverage | 18.98% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.84% |
| Branch coverage | 19.01% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
Please tag the upstream PR in the description for future reference. |
done |
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.84% |
| Branch coverage | 19.00% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
| boolean allFlash = Boolean.TRUE.equals(node.getAllFlashOptimized()); | ||
| boolean capacity = Boolean.TRUE.equals(node.getCapacityOptimized()); | ||
| if (!allFlash) { | ||
| if (node.getAllFlashOptimized() == null) { |
There was a problem hiding this comment.
i think this null check is not required as value will always come , also if we want to keep null check , we can keep for node.getCapacityOptimized() also. Harmless code change , can take it as part of any other PR also.
There was a problem hiding this comment.
yes, this value can never be null, it will always be true or false.
3 participants
Description
Upstream PR : apache#14193
1- This allocates an intermediate list and then traverses it three more times. Since this runs once per pool per interval, on very large pools this adds avoidable CPU/GC overhead. Consider computing rootDiskCount, dataDiskCount, and totalLogicalSizeBytes in a single pass over volumes (filtering by CS_VOLUME_STATES inline) to reduce allocations and repeated streaming.
2- This treats a missing/unknown is_all_flash_optimized (null) as false, which classifies the node as fas and can incorrectly set cluster platformType when ONTAP doesn’t return these fields (permissions/older versions/partial responses). Consider returning null (unknown) when the required flags are absent and only adding non-null classifications to the rollup set so ASUP doesn’t misreport hardware type.
3- The DAO method is documented as “non-destroyed” volumes, but the SQL currently includes volumes in transitional/pre-provisioning states (e.g., Allocated) because it only excludes Destroy/Expunged. That can incorrectly flip multiPrimaryStoragePoolVm=true for VMs whose data disk hasn’t actually been created/moved yet. Tighten the state predicates to align with the intended “physically exists” semantics (e.g., include only the same stable states you count elsewhere, or explicitly exclude Allocated/upload-related states).
4- lock.unlock() is executed even when lock.lock(...) returns false (early return), which can attempt to unlock a lock this thread never acquired. Additionally, GlobalLock.getInternLock(...) typically requires releasing the reference (e.g., releaseRef()) to avoid leaking the interned lock object. Track a boolean locked and only unlock() when true, and ensure the lock reference is released in finally (even when not acquired).
fixed the above issues
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?