Copilot commented on code in PR #12874:
URL: https://github.com/apache/cloudstack/pull/12874#discussion_r3008226929


##########
plugins/hypervisors/simulator/src/main/java/com/cloud/resource/SimulatorDiscoverer.java:
##########
@@ -20,8 +20,10 @@
 import java.net.URLDecoder;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.HashSet;

Review Comment:
   SimulatorDiscoverer no longer uses java.util.Date after switching to 
TemplateService-based association, but the Date import remains. Unused imports 
are compilation errors in Java; please remove this import (and any other 
now-unused imports introduced by this refactor).
   ```suggestion
   import java.util.HashMap;
   ```



##########
plugins/hypervisors/simulator/src/main/java/com/cloud/resource/SimulatorDiscoverer.java:
##########
@@ -50,6 +52,7 @@
 import com.cloud.storage.VMTemplateZoneVO;
 import com.cloud.storage.dao.VMTemplateDao;
 import com.cloud.storage.dao.VMTemplateZoneDao;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService;

Review Comment:
   After refactoring to call TemplateService in postDiscovery, the VMTemplate* 
imports are no longer used in this class. These unused imports will fail 
compilation; remove them (and consider dropping the corresponding unused 
injected fields as well).



##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4232,7 +4232,6 @@ private void duplicateCacheStoreRecordsToRegionStore(long 
storeId) {
     private void associateCrosszoneTemplatesToZone(Long zoneId) {
         VMTemplateZoneVO tmpltZone;

Review Comment:
   This helper is still named `associateCrosszoneTemplatesToZone` while the 
TemplateService API and most call sites now use 
`associateCrossZoneTemplatesToZone`. Renaming this private method (and its call 
at StorageManagerImpl.java:4004) would keep naming consistent and make grepping 
for the behavior easier.
   ```suggestion
   
   ```



##########
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java:
##########
@@ -285,23 +290,24 @@ public void postDiscovery(List<HostVO> hosts, long msId) {
                 _agentMgr.agentStatusTransitTo(h, Event.AgentDisconnected, 
msId);
             }
         }
+        Set<Long> dcIds = new HashSet<>();
         for (HostVO h : hosts) {
-            associateTemplatesToZone(h.getId(), h.getDataCenterId());
+            dcIds.add(h.getDataCenterId());
+        }
+        for (Long dcId : dcIds) {
+            templateService.associateCrossZoneTemplatesToZone(dcId);
         }
-
     }
 
     private void associateTemplatesToZone(long hostId, long dcId) {
         VMTemplateZoneVO tmpltZone;
 
-        List<VMTemplateVO> allTemplates = _vmTemplateDao.listAll();
-        for (VMTemplateVO vt : allTemplates) {
-            if (vt.isCrossZones()) {
-                tmpltZone = _vmTemplateZoneDao.findByZoneTemplate(dcId, 
vt.getId());
-                if (tmpltZone == null) {
-                    VMTemplateZoneVO vmTemplateZone = new 
VMTemplateZoneVO(dcId, vt.getId(), new Date());
-                    _vmTemplateZoneDao.persist(vmTemplateZone);
-                }
+        List<VMTemplateVO> crossZoneTemplates = 
_vmTemplateDao.listAllCrossZoneTemplates();
+        for (VMTemplateVO vt : crossZoneTemplates) {
+            tmpltZone = _vmTemplateZoneDao.findByZoneTemplate(dcId, 
vt.getId());
+            if (tmpltZone == null) {
+                VMTemplateZoneVO vmTemplateZone = new VMTemplateZoneVO(dcId, 
vt.getId(), new Date());
+                _vmTemplateZoneDao.persist(vmTemplateZone);
             }
         }
     }

Review Comment:
   postDiscovery now delegates cross-zone association to TemplateService, but 
the private associateTemplatesToZone(...) helper remains and is no longer 
called (and its hostId parameter is unused). Removing this unused method (and 
the now-redundant VMTemplate* DAO dependencies it exists solely to support) 
will reduce dead code and avoid maintaining two association paths.



##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -4232,7 +4232,6 @@ private void duplicateCacheStoreRecordsToRegionStore(long 
storeId) {
     private void associateCrosszoneTemplatesToZone(Long zoneId) {
         VMTemplateZoneVO tmpltZone;

Review Comment:
   associateCrosszoneTemplatesToZone now just delegates to 
_imageSrv.associateCrossZoneTemplatesToZone, so the local variable `tmpltZone` 
is no longer used. Please remove the unused variable to keep the method clean 
(and avoid IDE/compiler warnings).
   ```suggestion
   
   ```



-- 
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]

Reply via email to