winterhazel commented on PR #13053: URL: https://github.com/apache/cloudstack/pull/13053#issuecomment-4833250961
> We have responded to all the comments Thanks, the map-related ones make sense, so I resolved them. Also, I have submitted a reply to https://github.com/apache/cloudstack/pull/13053#discussion_r3204140918 if you could have a look. > @winterhazel Correct me if this is what you are trying to suggest as per my understanding on this comment [#13053 (comment)](https://github.com/apache/cloudstack/pull/13053#discussion_r3223364310) - We are doing volumeInfo.get_iScsiName() in VolumeServiceImpl.createManagedVolumeCopyTemplateAsync whose value is set during OntapPrimaryDatastoreDriver.grantAccess from db. Instead of setting details.put(PrimaryDataStore.MANAGED_STORE_TARGET, managedStoreTarget) using that value, set the PrimaryDataStore.MANAGED_STORE_TARGET in OntapPrimaryDatastoreDriver.grantAccess itself. This will save one db call happening in VolumeServiceImpl.createManagedVolumeCopyTemplateAsync right ? Right, this is what I suggested initially. > If yes, I had 2 reasons of not pursuing that- > > 1. Since the details is Map<String,String> and MANAGED_STORE_TARGET and other keys is specific(used by) by core code instead of vendor, vendor need not to remember the key (Also having key as String type can be error prone by vendors). > > 2. Existing code 1323-1330 also set the details here instead of any storage vendor setting those in either createAsync or grantAccess. > > > My understanding was that the vendor should be responsible for updating the right data in volume fields like iscsi_name, path , pool_type etc instead of setting key,value in details map which can be used by orchestrator code. Your points make sense from an architectural view, but we've got an exception here anyways in which the core will need to perform an extra step for all providers to set this detail due to how a single vendor works. Alternatively, can we move `grantAccess(volumeInfo, destHost, primaryDataStore);` to a line before `Map<String, String> details = new HashMap<>();` and replace the current L1328 with L1346 then (also remove the current L1344-L1348)? This in-memory details is not used by any provider, only for the final `CopyCommand`. This way, we do not have to perform this extra step. -- 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]
