Edison, The ConfigurationManagerImpl import change was result of a merge conflict resolution. During that process, I noticed that the imports didn't conform to the projects guidelines, so I organized them as part of the resolution process.
I have resolved the issue with the pom.xml change. The change submitted was the result of an upstream merge conflict resolution. However, as I reviewed the changes in my branch and master, the content was identical. Therefore, I reverted pom.xml in my branch to master, regenerated the patch, and submitted it via review board [1]. Thanks again for your help, -John [1] https://reviews.apache.org/r/8123/ On Dec 19, 2012, at 4:58 PM, Edison Su <edison...@citrix.com> wrote: > If I click https://reviews.apache.org/r/8123/diff/5/, I get: > The patch to 'pom.xml' didn't apply cleanly. The temporary files have been > left in '/tmp/reviewboard.gkgGgx' for debugging purposes. `patch` returned: > patching file /tmp/reviewboard.gkgGgx/tmpvi_JO8 Reversed (or previously > applied) patch detected! Assume -R? [n] Apply anyway? [n] Skipping patch. 1 > out of 1 hunk ignored -- saving rejects to file > /tmp/reviewboard.gkgGgx/tmpvi_JO8-new.rej > > In diff5, there is lot of changes on java import in ConfigurationManagerImpl, > while there is no code change in the class itself, > > Something like: > +++ b/server/src/com/cloud/configuration/ConfigurationManagerImpl.java > @@ -16,35 +16,110 @@ > // under the License. > package com.cloud.configuration; > > +import java.net.URI; > +import java.sql.PreparedStatement; > +import java.sql.ResultSet; > +import java.sql.SQLException; > +import java.util.ArrayList; > +import java.util.Collection; > +import java.util.HashMap; > +import java.util.HashSet; > +import java.util.Hashtable; > +import java.util.Iterator; > +import java.util.List; > +import java.util.Map; > +import java.util.Set; > +import java.util.UUID; > + > +import javax.ejb.Local; > +import javax.naming.ConfigurationException; > +import javax.naming.Context; > +import javax.naming.NamingException; > +import javax.naming.directory.DirContext; > +import javax.naming.directory.InitialDirContext; > + > +import org.apache.log4j.Logger; > + > import com.cloud.acl.SecurityChecker; > import com.cloud.alert.AlertManager; > import com.cloud.api.ApiConstants.LDAPParams; > import com.cloud.api.ApiDBUtils; > -import com.cloud.api.commands.*; > +import com.cloud.api.commands.CreateDiskOfferingCmd; > +import com.cloud.api.commands.CreateNetworkOfferingCmd; > +import com.cloud.api.commands.CreateServiceOfferingCmd; > +import com.cloud.api.commands.CreateVlanIpRangeCmd; > +import com.cloud.api.commands.CreateZoneCmd; > +import com.cloud.api.commands.DeleteDiskOfferingCmd; > +import com.cloud.api.commands.DeleteNetworkOfferingCmd; > +import com.cloud.api.commands.DeletePodCmd; > +import com.cloud.api.commands.DeleteServiceOfferingCmd; > +import com.cloud.api.commands.DeleteVlanIpRangeCmd; > +import com.cloud.api.commands.DeleteZoneCmd; > +import com.cloud.api.commands.LDAPConfigCmd; > +import com.cloud.api.commands.LDAPRemoveCmd; > +import com.cloud.api.commands.ListNetworkOfferingsCmd; > +import com.cloud.api.commands.UpdateCfgCmd; > +import com.cloud.api.commands.UpdateDiskOfferingCmd; > +import com.cloud.api.commands.UpdateNetworkOfferingCmd; > +import com.cloud.api.commands.UpdatePodCmd; > +import com.cloud.api.commands.UpdateServiceOfferingCmd; > +import com.cloud.api.commands.UpdateZoneCmd; > import com.cloud.capacity.dao.CapacityDao; > import com.cloud.configuration.Resource.ResourceType; > import com.cloud.configuration.dao.ConfigurationDao; > -import com.cloud.dc.*; > +import com.cloud.dc.AccountVlanMapVO; > +import com.cloud.dc.ClusterVO; > +import com.cloud.dc.DataCenter; > import com.cloud.dc.DataCenter.NetworkType; > +import com.cloud.dc.DataCenterIpAddressVO; > +import com.cloud.dc.DataCenterLinkLocalIpAddressVO; > +import com.cloud.dc.DataCenterVO; > +import com.cloud.dc.HostPodVO; > +import com.cloud.dc.Pod; > +import com.cloud.dc.PodVlanMapVO; > +import com.cloud.dc.Vlan; > import com.cloud.dc.Vlan.VlanType; > -import com.cloud.dc.dao.*; > +import com.cloud.dc.VlanVO; > +import com.cloud.dc.dao.AccountVlanMapDao; > +import com.cloud.dc.dao.ClusterDao; > +import com.cloud.dc.dao.DataCenterDao; > +import com.cloud.dc.dao.DataCenterIpAddressDao; > +import com.cloud.dc.dao.DataCenterLinkLocalIpAddressDaoImpl; > +import com.cloud.dc.dao.HostPodDao; > +import com.cloud.dc.dao.PodVlanMapDao; > +import com.cloud.dc.dao.VlanDao; > import com.cloud.deploy.DataCenterDeployment; > import com.cloud.domain.Domain; > import com.cloud.domain.DomainVO; > import com.cloud.domain.dao.DomainDao; > import com.cloud.event.ActionEvent; > import com.cloud.event.EventTypes; > -import com.cloud.exception.*; > +import com.cloud.exception.ConcurrentOperationException; > +import com.cloud.exception.InsufficientCapacityException; > +import com.cloud.exception.InvalidParameterValueException; > +import com.cloud.exception.PermissionDeniedException; > +import com.cloud.exception.ResourceAllocationException; > +import com.cloud.exception.ResourceUnavailableException; > import com.cloud.host.HostVO; > import com.cloud.hypervisor.Hypervisor.HypervisorType; > -import com.cloud.network.*; > +import com.cloud.network.IPAddressVO; > +import com.cloud.network.Network; > import com.cloud.network.Network.Capability; > import com.cloud.network.Network.GuestType; > import com.cloud.network.Network.Provider; > import com.cloud.network.Network.Service; > +import com.cloud.network.NetworkManager; > +import com.cloud.network.NetworkVO; > import com.cloud.network.Networks.BroadcastDomainType; > import com.cloud.network.Networks.TrafficType; > -import com.cloud.network.dao.*; > +import com.cloud.network.PhysicalNetwork; > +import com.cloud.network.PhysicalNetworkVO; > +import com.cloud.network.dao.FirewallRulesDao; > +import com.cloud.network.dao.IPAddressDao; > +import com.cloud.network.dao.NetworkDao; > +import com.cloud.network.dao.PhysicalNetworkDao; > +import com.cloud.network.dao.PhysicalNetworkTrafficTypeDao; > +import com.cloud.network.dao.PhysicalNetworkTrafficTypeVO; > import com.cloud.network.vpc.VpcManager; > import com.cloud.offering.DiskOffering; > import com.cloud.offering.NetworkOffering; > > > Are all these imports are necessary? > > > >> -----Original Message----- >> From: John Burwell [mailto:nore...@reviews.apache.org] On Behalf Of John >> Burwell >> Sent: Wednesday, December 19, 2012 1:40 PM >> To: Edison Su >> Cc: cloudstack; John Burwell >> Subject: Re: Review Request: S3-backed Secondary Storage >> >> >> >>> On Dec. 17, 2012, 5:51 p.m., edison su wrote: >>>> Hi John, thanks for the patch. Few comments: 1. I can't apply the patch >> against master top, there is conflict in pom.xml. 2. Could you remove the >> unnecessary change in ConfigurationManagerImpl file? >> >> Edison, >> >> Which change in ConfigurationManagerImpl is unnecessary? Looking >> through the diff, I cant find any unnecessary changes. Also, can you specify >> the merge issue you are encountering with pom.xml? I just merged my >> feature branch with cloudstack/master and had no conflicts. >> >> Thanks, >> -John >> >> >> - John >> >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/8123/#review14588 >> ----------------------------------------------------------- >> >> >> On Dec. 14, 2012, 11:54 p.m., John Burwell wrote: >>> >>> ----------------------------------------------------------- >>> This is an automatically generated e-mail. To reply, visit: >>> https://reviews.apache.org/r/8123/ >>> ----------------------------------------------------------- >>> >>> (Updated Dec. 14, 2012, 11:54 p.m.) >>> >>> >>> Review request for cloudstack and edison su. >>> >>> >>> Description >>> ------- >>> >>> Backs NFS-based secondary storage with an S3-compatible object store. >> Periodically, a reaper thread synchronizes templates and ISOs stored on a >> NFS secondary storage mount with a configured S3 object store. It also >> pushes snapshots to the object store when they are created and downloads >> them in other zones on-demand. In addition to permitting the use of >> commodity or IaaS storage solutions for static assets, it provides a means of >> automatically synchronizing template and ISO assets across multiple zones. >>> >>> For more information about the design of the patch, please see the design >> document (https://cwiki.apache.org/confluence/display/CLOUDSTACK/S3- >> backed+Secondary+Storage). >>> >>> >>> This addresses bug CLOUDSTACK-509. >>> >>> >>> Diffs >>> ----- >>> >>> api/src/com/cloud/api/commands/ListS3sCmd.java PRE-CREATION >>> build/package.xml 09ed939 >>> pom.xml 4a4276e >>> server/src/com/cloud/configuration/ConfigurationManagerImpl.java >> ef940e8 >>> server/src/com/cloud/server/ManagementServerImpl.java 117be57 >>> server/src/com/cloud/storage/dao/VMTemplateDao.java f5b6913 >>> server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 2a0dfc8 >>> server/src/com/cloud/storage/s3/S3ManagerImpl.java PRE-CREATION >>> tools/marvin/marvin/cloudstackConnection.py c805213 >>> tools/marvin/marvin/deployDataCenter.py bdf08cc >>> utils/src/com/cloud/utils/db/DbUtil.java feef7b3 >>> >>> Diff: https://reviews.apache.org/r/8123/diff/ >>> >>> >>> Testing >>> ------- >>> >>> I am submitting patch to begin the feedback process while we complete >> integration testing. I have verified that it does not interfere with normal >> CloudStack operations when S3-backed Secondary Storage is disabled (the >> default setting) . I have successfully tested operation of single zone >> template and ISO scenarios on devcloud described in the design document. I >> am currently working through some issues in our multi-zone test >> environment to complete all scenarios described. The following are the >> known deficiencies of the current implementation which I plan to correct in a >> subsequent patch: >>> >>> * Cross zone garbage collection: When a global asset is deleted from one >> zone's secondary storage, it is not deleted from the secondary storage of >> other zones which have downloaded it from the object store >>> * S3 Configuration Update: The API only supports adding an object store >> configuration. Users should be able to edit the access key, secret key, >> connection timeout. max error retries, and socket timeout. >>> * Multi-threaded Uploads: Permit the upload of multiple assets to the >> object store simultaneously to decrease the propagation latency across all >> zones. >>> >>> >>> Screenshots >>> ----------- >>> >>> S3 Configuration Form >>> https://reviews.apache.org/r/8123/s/13/ >>> S3 Enable Menu on the Zone Tab >>> https://reviews.apache.org/r/8123/s/14/ >>> >>> >>> Thanks, >>> >>> John Burwell >>> >>> >
