Return-Path: 
 <dev-return-89200-apmail-cloudstack-dev-archive=cloudstack.apache.org@cloudstack.apache.org>
X-Original-To: apmail-cloudstack-dev-archive@www.apache.org
Delivered-To: apmail-cloudstack-dev-archive@www.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 87641189BD
	for <apmail-cloudstack-dev-archive@www.apache.org>;
 Mon,  1 Feb 2016 01:22:22 +0000 (UTC)
Received: (qmail 82213 invoked by uid 500); 1 Feb 2016 01:22:22 -0000
Delivered-To: apmail-cloudstack-dev-archive@cloudstack.apache.org
Received: (qmail 82146 invoked by uid 500); 1 Feb 2016 01:22:22 -0000
Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@cloudstack.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@cloudstack.apache.org>
List-Post: <mailto:dev@cloudstack.apache.org>
List-Id: <dev.cloudstack.apache.org>
Reply-To: dev@cloudstack.apache.org
Delivered-To: mailing list dev@cloudstack.apache.org
Received: (qmail 82135 invoked by uid 99); 1 Feb 2016 01:22:21 -0000
Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org)
 (140.211.11.23)
    by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 01 Feb 2016 01:22:21 +0000
Received: by git1-us-west.apache.org (ASF Mail Server at
 git1-us-west.apache.org, from userid 33)
	id CDECFDFF85; Mon,  1 Feb 2016 01:22:21 +0000 (UTC)
From: pedro-martins <git@git.apache.org>
To: dev@cloudstack.apache.org
Reply-To: dev@cloudstack.apache.org
References: <git-pr-556-cloudstack@git.apache.org>
In-Reply-To: <git-pr-556-cloudstack@git.apache.org>
Subject: [GitHub] cloudstack pull request: CLOUDSTACK-8609. [VMware] VM is not
 acces...
Content-Type: text/plain
Message-Id: <20160201012221.CDECFDFF85@git1-us-west.apache.org>
Date: Mon,  1 Feb 2016 01:22:21 +0000 (UTC)

Github user pedro-martins commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/556#discussion_r51372149
  
    --- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java ---
    @@ -1765,7 +1767,18 @@ protected StartAnswer execute(StartCommand cmd) {
     
                 // Since VM was successfully powered-on, if there was an existing VM in a different cluster that was unregistered, delete all the files associated with it.
                 if (existingVmName != null && existingVmFileLayout != null) {
    -                deleteUnregisteredVmFiles(existingVmFileLayout, dcMo, true);
    +                List<String> skipDatastores = new ArrayList<String>();
    +                List<String> vmDatastoreNames = new ArrayList<String>();
    +                for (DatastoreMO vmDatastore : vmMo.getAllDatastores()) {
    +                    vmDatastoreNames.add(vmDatastore.getName());
    +                }
    +                // Don't delete files that are in a datastore that is being used by the new VM as well (zone-wide datastore).
    +                for (DatastoreMO existingDatastore : existingDatastores) {
    +                    if (vmDatastoreNames.contains(existingDatastore.getName())) {
    +                        skipDatastores.add(existingDatastore.getName());
    +                    }
    +                }
    +                deleteUnregisteredVmFiles(existingVmFileLayout, dcMo, true, skipDatastores);
                 }
    --- End diff --
    
    Hi, likitha.
    Could you extract the code between lines 1770 - 1775 and 1776 - 1780 to methods with a little test case and Javadoc explaining what the methods do, the params that they use and what they return?
    
    Another thing, how about you replace the logic in 'if's at lines 2272, 2283 and 2291 for a method with a little test case and a Javadoc, that receives 2 params, (List<String> skipDatastores, String name ) and returns  skipDatastores == null || !skipDatastores.contains(name).
    
    Ty.


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