Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
From: jburwell <git@git.apache.org>
To: dev@cloudstack.apache.org
Reply-To: dev@cloudstack.apache.org
References: <git-pr-1403-cloudstack@git.apache.org>
In-Reply-To: <git-pr-1403-cloudstack@git.apache.org>
Subject: [GitHub] cloudstack pull request: Taking fast and efficient volume
 snapshot...
Content-Type: text/plain
Message-Id: <20160408194753.F0F05DFC6F@git1-us-west.apache.org>
Date: Fri,  8 Apr 2016 19:47:53 +0000 (UTC)

Github user jburwell commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1403#discussion_r59079506
  
    --- Diff: engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java ---
    @@ -361,59 +608,225 @@ private String getProperty(long snapshotId, String property) {
         }
     
         private Map<String, String> getVolumeDetails(VolumeInfo volumeInfo) {
    -        Map<String, String> sourceDetails = new HashMap<String, String>();
    +        Map<String, String> volumeDetails = new HashMap<String, String>();
     
             VolumeVO volumeVO = _volumeDao.findById(volumeInfo.getId());
     
             long storagePoolId = volumeVO.getPoolId();
             StoragePoolVO storagePoolVO = _storagePoolDao.findById(storagePoolId);
     
    -        sourceDetails.put(DiskTO.STORAGE_HOST, storagePoolVO.getHostAddress());
    -        sourceDetails.put(DiskTO.STORAGE_PORT, String.valueOf(storagePoolVO.getPort()));
    -        sourceDetails.put(DiskTO.IQN, volumeVO.get_iScsiName());
    +        volumeDetails.put(DiskTO.STORAGE_HOST, storagePoolVO.getHostAddress());
    +        volumeDetails.put(DiskTO.STORAGE_PORT, String.valueOf(storagePoolVO.getPort()));
    +        volumeDetails.put(DiskTO.IQN, volumeVO.get_iScsiName());
     
             ChapInfo chapInfo = _volumeService.getChapInfo(volumeInfo, volumeInfo.getDataStore());
     
             if (chapInfo != null) {
    -            sourceDetails.put(DiskTO.CHAP_INITIATOR_USERNAME, chapInfo.getInitiatorUsername());
    -            sourceDetails.put(DiskTO.CHAP_INITIATOR_SECRET, chapInfo.getInitiatorSecret());
    -            sourceDetails.put(DiskTO.CHAP_TARGET_USERNAME, chapInfo.getTargetUsername());
    -            sourceDetails.put(DiskTO.CHAP_TARGET_SECRET, chapInfo.getTargetSecret());
    +            volumeDetails.put(DiskTO.CHAP_INITIATOR_USERNAME, chapInfo.getInitiatorUsername());
    +            volumeDetails.put(DiskTO.CHAP_INITIATOR_SECRET, chapInfo.getInitiatorSecret());
    +            volumeDetails.put(DiskTO.CHAP_TARGET_USERNAME, chapInfo.getTargetUsername());
    +            volumeDetails.put(DiskTO.CHAP_TARGET_SECRET, chapInfo.getTargetSecret());
    +        }
    +
    +        return volumeDetails;
    +    }
    +
    +    private Map<String, String> getSnapshotDetails(SnapshotInfo snapshotInfo) {
    +        Map<String, String> snapshotDetails = new HashMap<String, String>();
    +
    +        long storagePoolId = snapshotInfo.getDataStore().getId();
    +        StoragePoolVO storagePoolVO = _storagePoolDao.findById(storagePoolId);
    +
    +        snapshotDetails.put(DiskTO.STORAGE_HOST, storagePoolVO.getHostAddress());
    +        snapshotDetails.put(DiskTO.STORAGE_PORT, String.valueOf(storagePoolVO.getPort()));
    +
    +        long snapshotId = snapshotInfo.getId();
    +
    +        snapshotDetails.put(DiskTO.IQN, getProperty(snapshotId, DiskTO.IQN));
    +
    +        snapshotDetails.put(DiskTO.CHAP_INITIATOR_USERNAME, getProperty(snapshotId, DiskTO.CHAP_INITIATOR_USERNAME));
    +        snapshotDetails.put(DiskTO.CHAP_INITIATOR_SECRET, getProperty(snapshotId, DiskTO.CHAP_INITIATOR_SECRET));
    +        snapshotDetails.put(DiskTO.CHAP_TARGET_USERNAME, getProperty(snapshotId, DiskTO.CHAP_TARGET_USERNAME));
    +        snapshotDetails.put(DiskTO.CHAP_TARGET_SECRET, getProperty(snapshotId, DiskTO.CHAP_TARGET_SECRET));
    +
    +        return snapshotDetails;
    +    }
    +
    +    private HostVO getHost(SnapshotInfo snapshotInfo) {
    +        HostVO hostVO = getHost(snapshotInfo.getDataCenterId(), true);
    +
    +        if (hostVO == null) {
    +            hostVO = getHost(snapshotInfo.getDataCenterId(), false);
    +
    +            if (hostVO == null) {
    +                throw new CloudRuntimeException("Unable to locate an applicable host");
    +            }
             }
     
    -        return sourceDetails;
    +        return hostVO;
         }
     
    -    public HostVO getHost(long dataStoreId) {
    -        StoragePoolVO storagePoolVO = _storagePoolDao.findById(dataStoreId);
    +    private HostVO getHost(Long zoneId, boolean computeClusterMustSupportResign) {
    +        if (zoneId == null) {
    +            throw new CloudRuntimeException("Zone ID cannot be null.");
    +        }
     
    -        List<? extends Cluster> clusters = _mgr.searchForClusters(storagePoolVO.getDataCenterId(), new Long(0), Long.MAX_VALUE, HypervisorType.XenServer.toString());
    +        List<? extends Cluster> clusters = _mgr.searchForClusters(zoneId, new Long(0), Long.MAX_VALUE, HypervisorType.XenServer.toString());
     
             if (clusters == null) {
    -            throw new CloudRuntimeException("Unable to locate an applicable cluster");
    +            clusters = new ArrayList<>();
             }
     
    +        Collections.shuffle(clusters, new Random(System.nanoTime()));
    --- End diff --
    
    ``new Random`` is expensive.  Did you consider initializing a ``Random`` instance as a static member passing the same instance into ``shuffle``?  Since ``Random`` is [thread-safe](http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6362070), the same instance could be safely used.  This would avoid the cost of seeding a RNG on every method call.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---