[GitHub] [cloudstack] harikrishna-patnala commented on a diff in pull request #6589: [Veeam] disable jobs but keep backups

["harikrishna-patnala (via GitHub)" <gi...@apache.org>]

harikrishna-patnala commented on code in PR #6589:
URL: https://github.com/apache/cloudstack/pull/6589#discussion_r1250404467


##########
engine/schema/src/main/resources/META-INF/db/schema-41720to41800.sql:
##########


Review Comment:
   These changes need to be moved to schema-41810to41900.sql. 



##########
api/src/main/java/org/apache/cloudstack/api/command/user/volume/DetachVolumeCmd.java:
##########


Review Comment:
   any reason for these setters in Cmd file ? if not can you please remove them.



##########
engine/schema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java:
##########
@@ -214,7 +214,7 @@ public NicVO findByNetworkIdAndMacAddress(long networkId, String mac) {
         SearchCriteria<NicVO> sc = AllFieldsSearch.create();
         sc.setParameters("network", networkId);
         sc.setParameters("macAddress", mac);
-        return findOneBy(sc);
+        return findOneIncludingRemovedBy(sc);

Review Comment:
   Is this an intended change for this PR? can you please help me understand why we are including removed entries. 
   Also there are other many usages of this method, so I see a chance of regression here.



##########
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java:
##########
@@ -538,13 +543,33 @@ private Long getPoolIdFromDatastoreUuid(String datastoreUuid) {
     /**
      * Get pool ID for disk
      */
-    private Long getPoolId(VirtualDisk disk) {
+    private Long getPoolId(VirtualDisk disk, long datacenterId, long clusterId) {
         VirtualDeviceBackingInfo backing = disk.getBacking();
         checkBackingInfo(backing);
         VirtualDiskFlatVer2BackingInfo info = (VirtualDiskFlatVer2BackingInfo)backing;
         String[] fileNameParts = info.getFileName().split(" ");
-        String datastoreUuid = StringUtils.substringBetween(fileNameParts[0], "[", "]");
-        return getPoolIdFromDatastoreUuid(datastoreUuid);
+        String datastore = StringUtils.substringBetween(fileNameParts[0], "[", "]");
+        if (UuidUtils.isUuid(datastore)) {
+            return getPoolIdFromDatastoreUuid(datastore);
+        }
+        return getPoolIdFromDatastoreNameOrPath(datastore, datacenterId, clusterId);
+    }
+
+    protected Long getPoolIdFromDatastoreNameOrPath(String datastore, long datacenterId, long clusterId) {

Review Comment:
   just to confirm, are we fixing any known issue here with respect to pool ID ?  I don't see this related to the backups



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org