Kunalbehbud commented on PR #13084: URL: https://github.com/apache/cloudstack/pull/13084#issuecomment-5065734687
Thanks for the review @nvazquez! **Unused overload** — good catch, removed. The two-arg `shouldForceFullCloneMigration` only existed so the tests could call it without threading a cache; the production path always passes the shared pool map. The tests now call the real three-arg signature. **Redundant DAO lookups** — this is already handled. `copyAsync` creates a single `Map<Long, StoragePoolVO> storagePoolsById` and passes it into `shouldForceFullCloneMigration`, and the main loop then reads through the same cache via `getStoragePool(...)`: ```java Map<Long, StoragePoolVO> storagePoolsById = new HashMap<>(); boolean forceFullCloneMigration = shouldForceFullCloneMigration(volumeDataStoreMap, destHost, storagePoolsById); ... StoragePoolVO destStoragePool = getStoragePool(storagePoolsById, destDataStore.getId()); StoragePoolVO sourceStoragePool = getStoragePool(storagePoolsById, srcVolumeInfo.getPoolId()); ``` `getStoragePool` only hits `_storagePoolDao.findById` on a cache miss, so each pool is fetched once per migration request rather than once per volume per pass — the DAO call count is actually lower than before this PR. **Copilot's test suggestion** — the test already stubs `skippedDirectDownloadVolume.isDirectDownload()` to `true` (via `lenient()`, since the skip means it is never reached) and additionally asserts `Mockito.verify(skippedDirectDownloadVolume, Mockito.never()).isDirectDownload()`. That is strictly stronger than the suggestion: it proves the volume was skipped *before* the direct-download check rather than merely that the result was `false`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
