GeezerVison

cloudstack-pull-analysis #278 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-rats #346 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-requests #1041 FAILURE
Looks like there's a problem with this pull request

@pedro-martins s_logger is a standard name for the variables and in each class it gets the .class as its log category. I fail to see how you change improves on this.
Also '4,076 additions and 3,379 deletions' means more code and not a reduction in code.
Finally I see a lot of reordering of variables in classes that I'd rather see in separate commits so the essentials of your change are easier reviewed.

@DaanHoogland I agree with your considerations. My first commit was not performed properly. Sadly, the eclipse ended up formatting classes I touched, and that is why the line number added was higher than the removed one.

I do understand that “s_ something“ is a proper way to instantiate a static variable in ACS classes. However, in few of the subclasses of “com.cloud.utils.component.ComponentLifecycleBase” it was used names such as “LOGGER” (that is a proper name for static field as JAVA standards), log, status_logger and others.

What we propose is to change the logger variable in “com.cloud.utils.component.ComponentLifecycleBase” to protected and remove its static and final declaration. Therefore we did the following in ComponentLifecycleBase:
protected Logger logger = Logger.getLogger(getClass());
This way, every single subclass of ComponentLifecycleBase, when instantiated would automatically have a logger instance for its proper class ready to be used.

I have edited the commit, what do you think of it now?

@pedro-martins your motivation makes sense to me, can you create a jira ticket and c&p it there? then use that as a tag for any commits (including cleanup/autoformat).

One concern remaining is (not knowing the entire hierarchy by head) whether instance level loggers will be any kind of overhead. I know there are a lot of singletons per management server instance but for for instance agents this is not true. Did you investigate this?

cloudstack-pull-rats #354 ABORTED

Got your comments, sadly we have not analyzed which classes are singleton or not.
We have opened a jira ticket to that: https://issues.apache.org/jira/browse/CLOUDSTACK-8750
If someone need a hand, just shoot an email.

cloudstack-pull-analysis #286 SUCCESS
This pull request looks good

Hi @DaanHoogland, after we opened the Jira Ticket, someone ended up assigning it to us!? So, we decided to take a lead and analyze the impact that the proposed changes may cause. It turns out that most of the classes of that hierarchic are singletons, therefore it would not have an impact on memory consumption. The result of the analysis is the following:

Beans instantiated with @component:

1. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
2. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java
3. /cloudstack/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java
4. /cloudstack/server/src/com/cloud/agent/manager/authn/impl/BasicAgentAuthManager.java
5. /cloudstack/plugins/network-elements/bigswitch/src/com/cloud/network/element/BigSwitchBcfElement.java
6. /cloudstack/plugins/network-elements/brocade-vcs/src/com/cloud/network/element/BrocadeVcsElement.java
7. /cloudstack/server/src/com/cloud/hypervisor/CloudZonesStartupProcessor.java
8. /cloudstack/server/src/com/cloud/alert/ClusterAlertAdapter.java
9. /cloudstack/engine/orchestration/src/com/cloud/cluster/agentlb/ClusterBasedAgentLoadBalancerPlanner.java
10. /cloudstack/server/src/com/cloud/alert/ConsoleProxyAlertAdapter.java
11. /cloudstack/plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailElementImpl.java
12. /cloudstack/plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVpcElementImpl.java
13. /cloudstack/plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ManagementNetworkGuru.java
14. /cloudstack/engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java
15. /cloudstack/server/src/com/cloud/resource/DummyHostDiscoverer.java
16. /cloudstack/plugins/network-elements/elastic-loadbalancer/src/com/cloud/network/element/ElasticLoadBalancerElement.java
17. /cloudstack/server/src/com/cloud/agent/manager/allocator/impl/FirstFitAllocator.java
18. /cloudstack/server/src/com/cloud/agent/manager/allocator/impl/RecreateHostAllocator.java
19. /cloudstack/plugins/network-elements/globodns/src/com/globo/globodns/cloudstack/element/GloboDnsElement.java
20. /cloudstack/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java
21. /cloudstack/plugins/network-elements/ovs/src/com/cloud/network/guru/OvsGuestNetworkGuru.java
22. /cloudstack/plugins/network-elements/vxlan/src/com/cloud/network/guru/VxlanGuestNetworkGuru.java
23. /cloudstack/plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java
24. /cloudstack/plugins/network-elements/nicira-nvp/src/com/cloud/network/element/NiciraNvpElement.java
25. /cloudstack/plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/OpendaylightElement.java
26. /cloudstack/plugins/host-allocators/random/src/com/cloud/agent/manager/allocator/impl/RandomAllocator.java
27. /cloudstack/server/src/com/cloud/ha/RecreatableFencer.java
28. /cloudstack/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java
29. /cloudstack/server/src/com/cloud/alert/SecondaryStorageVmAlertAdapter.java
30. /cloudstack/plugins/api/discovery/src/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java
31. /cloudstack/engine/schema/src/com/cloud/user/dao/AccountDaoImpl.java
32. /cloudstack/server/src/com/cloud/api/query/dao/AccountJoinDaoImpl.java
33. /cloudstack/engine/schema/src/com/cloud/capacity/dao/CapacityDaoImpl.java
34. /cloudstack/engine/schema/src/com/cloud/certificate/dao/CertificateDaoImpl.java
35. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/network/dao/CiscoNexusVSMDeviceDaoImpl.java
36. /cloudstack/framework/config/src/org/apache/cloudstack/framework/config/dao/ConfigurationDaoImpl.java
37. /cloudstack/engine/schema/src/com/cloud/vm/dao/ConsoleProxyDaoImpl.java
38. /cloudstack/engine/schema/src/com/cloud/dc/dao/DataCenterDaoImpl.java
39. /cloudstack/engine/schema/src/com/cloud/dc/dao/DataCenterIpAddressDaoImpl.java
40. /cloudstack/server/src/com/cloud/api/query/dao/DataCenterJoinDaoImpl.java
41. /cloudstack/engine/schema/src/com/cloud/dc/dao/DataCenterLinkLocalIpAddressDaoImpl.java
42. /cloudstack/server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java
43. /cloudstack/engine/schema/src/com/cloud/domain/dao/DomainDaoImpl.java
44. /cloudstack/server/src/com/cloud/api/query/dao/DomainJoinDaoImpl.java
45. /cloudstack/server/src/com/cloud/api/query/dao/DomainRouterJoinDaoImpl.java
46. /cloudstack/engine/orchestration/src/org/apache/cloudstack/engine/datacenter/entity/api/db/dao/EngineClusterDaoImpl.java
47. /cloudstack/engine/schema/src/com/cloud/event/dao/EventDaoImpl.java
48. /cloudstack/server/src/com/cloud/event/dao/EventJoinDaoImpl.java
49. /cloudstack/engine/schema/src/com/cloud/network/dao/FirewallRulesCidrsDaoImpl.java
50. /cloudstack/server/src/com/cloud/ha/dao/HighAvailabilityDaoImpl.java
51. /cloudstack/engine/schema/src/com/cloud/host/dao/HostDaoImpl.java
52. /cloudstack/engine/schema/src/com/cloud/gpu/dao/HostGpuGroupsDaoImpl.java
53. /cloudstack/server/src/com/cloud/api/query/dao/HostJoinDaoImpl.java
54. /cloudstack/engine/schema/src/com/cloud/dc/dao/HostPodDaoImpl.java
55. /cloudstack/server/src/com/cloud/api/query/dao/HostTagDaoImpl.java
56. /cloudstack/engine/schema/src/com/cloud/cluster/agentlb/dao/HostTransferMapDaoImpl.java
57. /cloudstack/engine/schema/src/com/cloud/hypervisor/dao/HypervisorCapabilitiesDaoImpl.java
58. /cloudstack/server/src/com/cloud/api/query/dao/ImageStoreJoinDaoImpl.java
59. /cloudstack/server/src/com/cloud/api/query/dao/InstanceGroupJoinDaoImpl.java
60. /cloudstack/engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java
61. /cloudstack/engine/schema/src/com/cloud/storage/dao/LaunchPermissionDaoImpl.java
62. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/dao/LegacyZoneDaoImpl.java
63. /cloudstack/plugins/file-systems/netapp/src/com/cloud/netapp/dao/LunDaoImpl.java
64. /cloudstack/engine/schema/src/com/cloud/network/vpc/dao/NetworkACLItemCidrsDaoImpl.java
65. /cloudstack/engine/schema/src/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java
66. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/db/ObjectInDataStoreDaoImpl.java
67. /cloudstack/plugins/file-systems/netapp/src/com/cloud/netapp/dao/PoolDaoImpl.java
68. /cloudstack/engine/schema/src/com/cloud/network/dao/PortProfileDaoImpl.java
69. /cloudstack/engine/schema/src/com/cloud/network/vpc/dao/PrivateIpDaoImpl.java
70. /cloudstack/engine/schema/src/com/cloud/projects/dao/ProjectAccountDaoImpl.java
71. /cloudstack/server/src/com/cloud/api/query/dao/ProjectAccountJoinDaoImpl.java
72. /cloudstack/engine/schema/src/com/cloud/projects/dao/ProjectDaoImpl.java
73. /cloudstack/engine/schema/src/com/cloud/projects/dao/ProjectInvitationDaoImpl.java
74. /cloudstack/server/src/com/cloud/api/query/dao/ProjectInvitationJoinDaoImpl.java
75. /cloudstack/server/src/com/cloud/api/query/dao/ProjectJoinDaoImpl.java
76. /cloudstack/engine/schema/src/org/apache/cloudstack/region/dao/RegionDaoImpl.java
77. /cloudstack/engine/schema/src/com/cloud/network/dao/RemoteAccessVpnDaoImpl.java
78. /cloudstack/engine/schema/src/com/cloud/network/dao/Site2SiteVpnConnectionDaoImpl.java
79. /cloudstack/engine/schema/src/com/cloud/network/dao/Site2SiteVpnGatewayDaoImpl.java
80. /cloudstack/engine/schema/src/com/cloud/storage/dao/SnapshotDaoImpl.java
81. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java
82. /cloudstack/engine/schema/src/com/cloud/storage/dao/StoragePoolHostDaoImpl.java
83. /cloudstack/server/src/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java
84. /cloudstack/server/src/com/cloud/api/query/dao/StorageTagDaoImpl.java
85. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java
86. /cloudstack/server/src/com/cloud/api/query/dao/TemplateJoinDaoImpl.java
87. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/volume/db/TemplatePrimaryDataStoreDaoImpl.java
88. /cloudstack/engine/schema/src/com/cloud/storage/dao/UploadDaoImpl.java
89. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageDaoImpl.java
90. /cloudstack/engine/schema/src/com/cloud/event/dao/UsageEventDaoImpl.java
91. /cloudstack/engine/schema/src/com/cloud/event/dao/UsageEventDetailsDaoImpl.java
92. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageIPAddressDaoImpl.java
93. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageJobDaoImpl.java
94. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageLoadBalancerPolicyDaoImpl.java
95. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageNetworkDaoImpl.java
96. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageNetworkOfferingDaoImpl.java
97. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsagePortForwardingRuleDaoImpl.java
98. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageSecurityGroupDaoImpl.java
99. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageStorageDaoImpl.java
100. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageVmDiskDaoImpl.java
101. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageVMInstanceDaoImpl.java
102. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageVMSnapshotDaoImpl.java
103. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageVolumeDaoImpl.java
104. /cloudstack/engine/schema/src/com/cloud/usage/dao/UsageVPNUserDaoImpl.java
105. /cloudstack/server/src/com/cloud/api/query/dao/UserAccountJoinDaoImpl.java
106. /cloudstack/engine/schema/src/com/cloud/network/dao/UserIpv6AddressDaoImpl.java
107. /cloudstack/engine/schema/src/com/cloud/user/dao/UserStatisticsDaoImpl.java
108. /cloudstack/engine/schema/src/com/cloud/vm/dao/UserVmCloneSettingDaoImpl.java
109. /cloudstack/server/src/com/cloud/api/query/dao/UserVmJoinDaoImpl.java
110. /cloudstack/engine/schema/src/com/cloud/upgrade/dao/VersionDaoImpl.java
111. /cloudstack/engine/schema/src/com/cloud/gpu/dao/VGPUTypesDaoImpl.java
112. /cloudstack/engine/schema/src/com/cloud/user/dao/VmDiskStatisticsDaoImpl.java
113. /cloudstack/engine/schema/src/org/apache/cloudstack/engine/cloud/entity/api/db/dao/VMEntityDaoImpl.java
114. /cloudstack/engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java
115. /cloudstack/engine/schema/src/com/cloud/network/security/dao/VmRulesetLogDaoImpl.java
116. /cloudstack/engine/schema/src/com/cloud/vm/snapshot/dao/VMSnapshotDaoImpl.java
117. /cloudstack/engine/schema/src/com/cloud/storage/dao/VMTemplateDaoImpl.java
118. /cloudstack/engine/schema/src/com/cloud/storage/dao/VMTemplateHostDaoImpl.java
119. /cloudstack/engine/schema/src/com/cloud/storage/dao/VMTemplatePoolDaoImpl.java
120. /cloudstack/engine/schema/src/com/cloud/storage/dao/VMTemplateZoneDaoImpl.java
121. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/dao/VmwareDatacenterDaoImpl.java
122. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/image/db/VolumeDataStoreDaoImpl.java
123. /cloudstack/engine/schema/src/com/cloud/storage/dao/VolumeHostDaoImpl.java
124. /cloudstack/server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java
125. /cloudstack/plugins/network-elements/ovs/src/com/cloud/network/ovs/dao/VpcDistributedRouterSeqNoDaoImpl.java
126. /cloudstack/server/src/org/apache/cloudstack/network/lb/ApplicationLoadBalancerManagerImpl.java
127. /cloudstack/framework/cluster/src/com/cloud/cluster/ClusterFenceManagerImpl.java
128. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/datastore/provider/DataStoreProviderManagerImpl.java
129. /cloudstack/server/src/com/cloud/user/DomainManagerImpl.java
130. /cloudstack/server/src/com/cloud/storage/download/DownloadMonitorImpl.java
131. /cloudstack/plugins/network-elements/elastic-loadbalancer/src/com/cloud/network/lb/ElasticLoadBalancerManagerImpl.java
132. /cloudstack/plugins/storage/volume/cloudbyte/src/org/apache/cloudstack/storage/datastore/util/ElastistorVolumeApiServiceImpl.java
133. /cloudstack/server/src/com/cloud/network/ExternalDeviceUsageManagerImpl.java
134. /cloudstack/server/src/com/cloud/network/ExternalNetworkDeviceManagerImpl.java
135. /cloudstack/server/src/com/cloud/network/firewall/FirewallManagerImpl.java
136. /cloudstack/server/src/com/cloud/hypervisor/HypervisorGuruManagerImpl.java
137. /cloudstack/server/src/com/cloud/storage/ImageStoreUploadMonitorImpl.java
138. /cloudstack/framework/security/src/org/apache/cloudstack/framework/security/keystore/KeystoreManagerImpl.java
139. /cloudstack/server/src/com/cloud/network/lb/LBHealthCheckManagerImpl.java
140. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/agent/manager/MockAgentManagerImpl.java
141. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/agent/manager/MockStorageManagerImpl.java
142. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/agent/manager/MockVmManagerImpl.java
143. /cloudstack/plugins/file-systems/netapp/src/com/cloud/netapp/NetappManagerImpl.java
144. /cloudstack/server/src/com/cloud/network/vpc/NetworkACLServiceImpl.java
145. /cloudstack/server/src/com/cloud/network/NetworkUsageManagerImpl.java
146. /cloudstack/server/src/com/cloud/storage/OCFS2ManagerImpl.java
147. /cloudstack/plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsTunnelManagerImpl.java
148. /cloudstack/server/src/com/cloud/projects/ProjectManagerImpl.java
149. /cloudstack/server/src/com/cloud/api/query/QueryManagerImpl.java
150. /cloudstack/server/src/org/apache/cloudstack/region/RegionManagerImpl.java
151. /cloudstack/server/src/org/apache/cloudstack/region/RegionServiceImpl.java
152. /cloudstack/server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
153. /cloudstack/server/src/com/cloud/resource/ResourceManagerImpl.java
154. /cloudstack/server/src/com/cloud/metadata/ResourceMetaDataManagerImpl.java
155. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/agent/manager/SimulatorManagerImpl.java
156. /cloudstack/server/src/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java
157. /cloudstack/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
158. /cloudstack/server/src/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java
159. /cloudstack/server/src/com/cloud/server/StatsCollector.java
160. /cloudstack/server/src/com/cloud/storage/StorageManagerImpl.java
161. /cloudstack/server/src/com/cloud/network/StorageNetworkManagerImpl.java
162. /cloudstack/server/src/com/cloud/storage/upload/UploadMonitorImpl.java
163. /cloudstack/usage/src/com/cloud/usage/UsageAlertManagerImpl.java
164. /cloudstack/usage/src/com/cloud/usage/UsageManagerImpl.java
165. /cloudstack/server/src/com/cloud/usage/UsageServiceImpl.java
166. /cloudstack/server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java
167. /cloudstack/server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java

Beans instantiated in Spring XMLs:

1. /cloudstack/server/src/com/cloud/ha/ManagementIPSystemVMInvestigator.java
2. /cloudstack/server/src/com/cloud/ha/UserVmDomRInvestigator.java
3. /cloudstack/server/src/com/cloud/ha/ManagementIPSystemVMInvestigator.java
4. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/allocator/GarbageCollectingStoragePoolAllocator.java
5. /cloudstack/plugins/affinity-group-processors/explicit-dedication/src/org/apache/cloudstack/affinity/ExplicitDedicationProcessor.java
6. /cloudstack/plugins/affinity-group-processors/host-anti-affinity/src/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java
7. /cloudstack/server/src/com/cloud/api/ApiAsyncJobDispatcher.java
8. /cloudstack/framework/jobs/test/org/apache/cloudstack/framework/jobs/AsyncJobTestDispatcher.java
9. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalDhcpElement.java
10. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/manager/BareMetalPlanner.java
11. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalPxeElement.java
12. /cloudstack/server/src/com/cloud/ha/CheckOnAgentInvestigator.java
13. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/network/element/CiscoNexusVSMElement.java
14. /cloudstack/plugins/network-elements/cisco-vnmc/src/com/cloud/network/element/CiscoVnmcElement.java
15. /cloudstack/server/src/com/cloud/network/element/CloudZonesNetworkElement.java
16. /cloudstack/framework/cluster/src/com/cloud/cluster/ClusterServiceServletAdapter.java
17. /cloudstack/plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailGuru.java
18. /cloudstack/plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java
19. /cloudstack/plugins/user-authenticators/md5/src/com/cloud/server/auth/MD5UserAuthenticator.java
20. /cloudstack/plugins/user-authenticators/pbkdf2/src/org/apache/cloudstack/server/auth/PBKDF2UserAuthenticator.java
21. /cloudstack/plugins/user-authenticators/plain-text/src/com/cloud/server/auth/PlainTextUserAuthenticator.java
22. /cloudstack/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2UserAuthenticator.java
23. /cloudstack/plugins/user-authenticators/sha256salted/src/com/cloud/server/auth/SHA256SaltedUserAuthenticator.java
24. /cloudstack/server/src/com/cloud/network/guru/DirectNetworkGuru.java
25. /cloudstack/server/src/com/cloud/network/guru/DirectPodBasedNetworkGuru.java
26. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetaNetworkGuru.java
27. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/manager/BareMetalDiscoverer.java
28. /cloudstack/plugins/hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/discoverer/HypervServerDiscoverer.java
29. /cloudstack/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3Discoverer.java
30. /cloudstack/plugins/hypervisors/ovm/src/com/cloud/ovm/hypervisor/OvmDiscoverer.java
31. /cloudstack/services/secondary-storage/server/src/org/apache/cloudstack/storage/resource/SecondaryStorageDiscoverer.java
32. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/resource/SimulatorSecondaryDiscoverer.java
33. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/resource/SimulatorDiscoverer.java
34. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/VmwareServerDiscoverer.java
35. /cloudstack/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/discoverer/XcpServerDiscoverer.java
36. /cloudstack/plugins/network-elements/juniper-srx/src/com/cloud/network/element/JuniperSRXExternalFirewallElement.java
37. /cloudstack/plugins/network-elements/palo-alto/src/com/cloud/network/element/PaloAltoExternalFirewallElement.java
38. /cloudstack/server/src/com/cloud/network/ExternalIpAddressAllocator.java
39. /cloudstack/plugins/network-elements/f5/src/com/cloud/network/element/F5ExternalLoadBalancerElement.java
40. /cloudstack/plugins/network-elements/netscaler/src/com/cloud/network/element/NetscalerElement.java
41. /cloudstack/plugins/network-elements/bigswitch/src/com/cloud/network/guru/BigSwitchBcfGuestNetworkGuru.java
42. /cloudstack/plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java
43. /cloudstack/server/src/com/cloud/network/guru/ExternalGuestNetworkGuru.java
44. /cloudstack/plugins/network-elements/nicira-nvp/src/com/cloud/network/guru/NiciraNvpGuestNetworkGuru.java
45. /cloudstack/plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
46. /cloudstack/plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/OpendaylightGuestNetworkGuru.java
47. /cloudstack/plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/network/guru/SspGuestNetworkGuru.java
48. /cloudstack/plugins/hypervisors/hyperv/src/com/cloud/ha/HypervInvestigator.java
49. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/manager/BareMetalGuru.java
50. /cloudstack/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java
51. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/guru/VMwareGuru.java
52. /cloudstack/plugins/network-elements/internal-loadbalancer/src/org/apache/cloudstack/network/element/InternalLoadBalancerElement.java
53. /cloudstack/server/src/com/cloud/ha/KVMFencer.java
54. /cloudstack/plugins/hypervisors/kvm/src/com/cloud/ha/KVMInvestigator.java
55. /cloudstack/plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java
56. /cloudstack/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3FenceBuilder.java
57. /cloudstack/plugins/hypervisors/ovm3/src/main/java/com/cloud/ha/Ovm3Investigator.java
58. /cloudstack/plugins/hypervisors/ovm/src/com/cloud/ovm/hypervisor/OvmFencer.java
59. /cloudstack/plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java
60. /cloudstack/server/src/com/cloud/deploy/FirstFitPlanner.java
61. /cloudstack/plugins/deployment-planners/implicit-dedication/src/com/cloud/deploy/ImplicitDedicationPlanner.java
62. /cloudstack/server/src/com/cloud/network/guru/PodBasedNetworkGuru.java
63. /cloudstack/server/src/com/cloud/network/guru/ControlNetworkGuru.java
64. /cloudstack/server/src/com/cloud/network/guru/StorageNetworkGuru.java
65. /cloudstack/server/src/com/cloud/network/guru/PrivateNetworkGuru.java
66. /cloudstack/server/src/com/cloud/network/guru/PublicNetworkGuru.java
67. /cloudstack/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetPublicNetworkGuru.java
68. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/ha/SimulatorFencer.java
69. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/ha/SimulatorInvestigator.java
70. /cloudstack/plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/network/element/SspElement.java
71. /cloudstack/plugins/acl/static-role-based/src/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java
72. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/manager/BareMetalTemplateAdapter.java
73. /cloudstack/server/src/com/cloud/template/HypervisorTemplateAdapter.java
74. /cloudstack/server/src/com/cloud/agent/manager/allocator/impl/UserConcentratedAllocator.java
75. /cloudstack/server/src/com/cloud/network/element/VirtualRouterElement.java
76. /cloudstack/server/src/com/cloud/network/element/VpcVirtualRouterElement.java
77. /cloudstack/engine/orchestration/src/com/cloud/vm/VmWorkJobDispatcher.java
78. /cloudstack/engine/orchestration/src/com/cloud/vm/VmWorkJobWakeupDispatcher.java
79. /cloudstack/plugins/hypervisors/xenserver/src/com/cloud/ha/XenServerFencer.java
80. /cloudstack/server/src/com/cloud/ha/XenServerInvestigator.java
81. /cloudstack/framework/spring/lifecycle/src/main/java/org/apache/cloudstack/spring/lifecycle/registry/DumpRegistry.java
82. /cloudstack/server/src/com/cloud/api/query/dao/AffinityGroupJoinDaoImpl.java
83. /cloudstack/engine/schema/src/com/cloud/vm/dao/UserVmDaoImpl.java
84. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/dao/VmWorkJobDaoImpl.java
85. /cloudstack/server/src/com/cloud/user/AccountManagerImpl.java
86. /cloudstack/server/src/org/apache/cloudstack/affinity/AffinityGroupServiceImpl.java
87. /cloudstack/server/src/com/cloud/consoleproxy/AgentBasedConsoleProxyManager.java
88. /cloudstack/engine/orchestration/src/com/cloud/agent/manager/AgentManagerImpl.java
89. /cloudstack/engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentManagerImpl.java
90. /cloudstack/server/src/com/cloud/alert/AlertManagerImpl.java
91. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java
92. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/impl/AsyncJobMonitor.java
93. /cloudstack/server/src/com/cloud/network/as/AutoScaleManagerImpl.java
94. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalDhcpManagerImpl.java
95. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/manager/BaremetalManagerImpl.java
96. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalPxeManagerImpl.java
97. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalKickStartServiceImpl.java
98. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BareMetalPingServiceImpl.java
99. /cloudstack/server/src/com/cloud/capacity/CapacityManagerImpl.java
100. /cloudstack/framework/cluster/src/com/cloud/cluster/ClusterManagerImpl.java
101. /cloudstack/server/src/com/cloud/configuration/ConfigurationManagerImpl.java
102. /cloudstack/server/src/com/cloud/server/ConfigurationServerImpl.java
103. /cloudstack/server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java
104. /cloudstack/plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailManagerImpl.java
105. /cloudstack/engine/storage/snapshot/src/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java
106. /cloudstack/server/src/com/cloud/deploy/DeploymentPlanningManagerImpl.java
107. /cloudstack/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/DirectAgentManagerSimpleImpl.java
108. /cloudstack/server/src/com/cloud/ha/HighAvailabilityManagerImpl.java
109. /cloudstack/server/src/com/cloud/ha/HighAvailabilityManagerExtImpl.java
110. /cloudstack/plugins/network-elements/internal-loadbalancer/src/org/apache/cloudstack/network/lb/InternalLoadBalancerVMManagerImpl.java
111. /cloudstack/server/src/com/cloud/network/IpAddressManagerImpl.java
112. /cloudstack/server/src/com/cloud/network/Ipv6AddressManagerImpl.java
113. /cloudstack/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
114. /cloudstack/server/src/com/cloud/server/ManagementServerImpl.java
115. /cloudstack/plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
116. /cloudstack/server/src/com/cloud/network/vpc/NetworkACLManagerImpl.java
117. /cloudstack/server/src/com/cloud/network/NetworkModelImpl.java
118. /cloudstack/server/src/com/cloud/network/NetworkServiceImpl.java
119. /cloudstack/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManagerImpl.java
120. /cloudstack/server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java
121. /cloudstack/server/src/com/cloud/network/rules/RulesManagerImpl.java
122. /cloudstack/services/secondary-storage/controller/src/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
123. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/impl/SyncQueueManagerImpl.java
124. /cloudstack/server/src/com/cloud/tags/TaggedResourceManagerImpl.java
125. /cloudstack/server/src/com/cloud/template/TemplateManagerImpl.java
126. /cloudstack/server/src/com/cloud/vm/UserVmManagerImpl.java
127. /cloudstack/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
128. /cloudstack/server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
129. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java
130. /cloudstack/server/src/com/cloud/storage/VolumeApiServiceImpl.java
131. /cloudstack/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
132. /cloudstack/server/src/com/cloud/network/vpc/VpcManagerImpl.java

Abstract classes:

1. /cloudstack/server/src/com/cloud/ha/AbstractInvestigatorImpl.java
2. /cloudstack/engine/storage/src/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java
3. /cloudstack/plugins/hypervisors/vmware/src/com/cloud/network/CiscoNexusVSMDeviceManagerImpl.java
4. /cloudstack/server/src/com/cloud/resource/DiscovererBase.java
5. /cloudstack/server/src/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
6. /cloudstack/server/src/com/cloud/network/ExternalFirewallDeviceManagerImpl.java
7. /cloudstack/server/src/com/cloud/network/ExternalLoadBalancerDeviceManagerImpl.java
8. /cloudstack/server/src/com/cloud/network/guru/GuestNetworkGuru.java
9. /cloudstack/server/src/com/cloud/hypervisor/HypervisorGuruBase.java
10. /cloudstack/server/src/com/cloud/template/TemplateAdapterBase.java
11. /cloudstack/framework/db/src/com/cloud/utils/db/GenericDaoBase.java
12. /cloudstack/server/src/com/cloud/api/ApiServer.java
13. /cloudstack/server/src/com/cloud/network/security/SecurityGroupManagerImpl2.java

Spring beans instantiated in XML and using @Local (EJB) annotation ?!!?:

1. /cloudstack/server/src/com/cloud/hypervisor/kvm/discoverer/KvmServerDiscoverer.java
2. /cloudstack/server/src/com/cloud/hypervisor/kvm/discoverer/LxcServerDiscoverer.java
3. /cloudstack/utils/src/org/apache/cloudstack/utils/identity/ManagementServerNode.java
4. /cloudstack/plugins/ha-planners/skip-heurestics/src/com/cloud/deploy/SkipHeuresticsPlanner.java
5. /cloudstack/plugins/deployment-planners/user-concentrated-pod/src/com/cloud/deploy/UserConcentratedPodPlanner.java
6. /cloudstack/plugins/deployment-planners/user-dispersing/src/com/cloud/deploy/UserDispersingPlanner.java
7. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/dao/AsyncJobDaoImpl.java
8. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/dao/AsyncJobJoinMapDaoImpl.java
9. /cloudstack/framework/cluster/src/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java
10. /cloudstack/framework/cluster/src/com/cloud/cluster/dao/ManagementServerHostPeerDaoImpl.java
11. /cloudstack/plugins/hypervisors/simulator/src/com/cloud/simulator/dao/MockConfigurationDaoImpl.java
12. /cloudstack/plugins/network-elements/stratosphere-ssp/src/org/apache/cloudstack/network/dao/SspUuidDaoImpl.java
13. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/dao/SyncQueueDaoImpl.java
14. /cloudstack/framework/jobs/src/org/apache/cloudstack/framework/jobs/dao/SyncQueueItemDaoImpl.java
15. /cloudstack/engine/schema/src/com/cloud/storage/dao/VolumeDaoImpl.java
16. /cloudstack/server/src/com/cloud/api/auth/APIAuthenticationManagerImpl.java
17. /cloudstack/server/test/com/cloud/vpc/MockNetworkManagerImpl.java

This class is never instantiated: /cloudstack/plugins/storage-allocators/random/src/org/apache/cloudstack/storage/allocator/RandomStoragePoolAllocator.java
We found a reference for a class under the same name, but different package in :/cloud-server/test/async-job-component.xml
However the full qualified name of the class is different: com.cloud.agent.manager.allocator.impl.RandomStoragePoolAllocator
We did not find the class that is being used in that XML

Others:

Instantiated only once at: org.apache.cloudstack.storage.template.DownloadManagerImpl.configure(String, Map<String, Object>)

1. /cloudstack/core/src/com/cloud/storage/template/IsoProcessor.java
2. /cloudstack/core/src/com/cloud/storage/template/OVAProcessor.java
3. /cloudstack/core/src/com/cloud/storage/template/QCOW2Processor.java
4. /cloudstack/core/src/com/cloud/storage/template/RawImageProcessor.java
5. /cloudstack/core/src/com/cloud/storage/template/TARProcessor.java
6. /cloudstack/core/src/com/cloud/storage/template/VhdProcessor.java
7. /cloudstack/core/src/com/cloud/storage/template/VmdkProcessor.java

Only used in tests:

1. /cloudstack/server/test/com/cloud/vpc/dao/MockVpcDaoImpl.java
2. /cloudstack/server/test/com/cloud/vpc/dao/MockNetworkOfferingDaoImpl.java

Never used:

1. /cloudstack/server/src/com/cloud/consoleproxy/AgentBasedStandaloneConsoleProxyManager.java
2. /cloudstack/plugins/event-bus/kafka/src/org/apache/cloudstack/mom/kafka/KafkaEventBus.java
3. /cloudstack/plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/RabbitMQEventBus.java

Instantiated when adding a new DHCP server

1. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalDhcpResourceBase.java
2. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalDhcpdResource.java
3. /cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalDnsmasqResource.java

Used only when adding a NuageVspDevice
/cloudstack/plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResource.java

Instantiated when adding pxeserver
/cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalPxeResourceBase.java
/cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalKickStartPxeResource.java
/cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetalPingPxeResource.java

Instantiated when adding baremetal cluster
/cloudstack/plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BareMetalResourceBase.java

Instantiated when adding BigSwitchBcf device
/cloudstack/plugins/network-elements/bigswitch/src/com/cloud/network/resource/BigSwitchBcfResource.java

Just used by Globo when adding a dns server
/cloudstack/plugins/network-elements/globodns/src/com/globo/globodns/cloudstack/resource/GloboDnsResource.java

Only used in test cases!?
/cloudstack/plugins/event-bus/inmemory/src/org/apache/cloudstack/mom/inmemory/InMemoryEventBus.java

Instantiated in a few different cases:
/cloudstack/services/secondary-storage/server/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
/cloudstack/services/secondary-storage/server/src/org/apache/cloudstack/storage/template/UploadManagerImpl.java

That was the full report. If you give the go ahead we can get the head revision execute our scripts and commit the changes. Would you re-open the PR?

PS: We found a little odd some spring beans with @Local annotation, we intend to investigate that later.

Sorry the huge post.

cloudstack-pull-rats #366 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-analysis #299 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-rats #367 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-analysis #300 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-rats #368 SUCCESS
This pull request looks good

cloudstack-pull-rats #369 SUCCESS
This pull request looks good

cloudstack-pull-rats #370 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-rats #371 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-analysis #301 SUCCESS
This pull request looks good

cloudstack-pull-analysis #302 SUCCESS
This pull request looks good

cloudstack-pull-analysis #303 UNSTABLE
Looks like there's a problem with this pull request

cloudstack-pull-analysis #304 UNSTABLE
Looks like there's a problem with this pull request

cloudstack-pull-rats #372 FAILURE
Looks like there's a problem with this pull request

cloudstack-pull-analysis #305 SUCCESS
This pull request looks good

As the system won't be build with just the first commit applied can you squash those two commits?

cloudstack-pull-rats #443 SUCCESS
This pull request looks good

cloudstack-pull-analysis #376 SUCCESS
This pull request looks good

LGTM

travis timeouts are unrelated. analysis passes, LGTM

Thanks for the hard work on reviewing this PR

jenkins noredist build failed with the below error. Reverting this PR

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:compile (default-compile) on project cloud-plugin-hypervisor-vmware: Compilation failure
[ERROR] /home/jenkins/acs/workspace/build-master-noredist/plugins/hypervisors/vmware/src/com/cloud/hypervisor/guru/VMwareGuru.java:[484,12] error: non-static variable logger cannot be referenced from a static context
[ERROR] -> [Help 1]

http://jenkins.buildacloud.org/job/build-master-noredist/4588/consoleText

Hi @karuturi, If you take a look at here:
https://github.com/rafaelweingartner/cloudstack/blob/master-lrg-cs-hackday-003/plugins/hypervisors/vmware/src/com/cloud/hypervisor/guru/VMwareGuru.java
You can see that the commit was properly coded. The method “resolveNameInGuid” was static (for some odd reason) and we had changed it to non-static, it does not need to be static. It seems that there was either a problem with the merge or someone else had put back the static keyword in that method there.

@karuturi, I was just reviewing my commit, I am really sorry.
I indeed forgot to commit the removal of the static keyword in that method. It was right in my workspace, but I had not committed it. Would you like us to re-open the PR to fix that? Or should we create a new one?

@mike-tutkowski @DaanHoogland @karuturi @miguelaferreira @rafaelweingartner @pedro-martins @remibergsma

First of all let me just get one thing clear: Rafael mentioned that he has a script that changes the Java files automatically. Did I read it right? If so, why doesn't this script also compile the code with a simple Maven run after the changes?

We have been suffering from things that simply break and are difficult to track because PRs are not properly reviewed/tested. In this particular case, how is possible to get 2 LGTM without even compiling the project locally?

Cheers,
Wilder

@wilderrodrigues I commented: "travis timeouts are unrelated. analysis passes, LGTM". So it was compiled otherwise the analysis would not have passed. What I missed there was the noredist build. This one must be run instead of the regular in cloudstack-pull-anaysis. hat job doesn't run at all atm. I made an infra ticket for that and next we need to (finaly) get the noredist in there. It had been on my list and was phased out of it. it is no back on :(

Hi @DaanHoogland, what I don't understand is how travis compiled and 15 days ago @rafaelweingartner said that he forgot to push 1 file.

Looking at the history, we see the following:

So, perhaps the timeout occurred before the missing file was needed, so one could not tell if it would have worked at all. Therefore we have to compile the PRs and test them before LGTM.

I also don't get why @mike-tutkowski simply gave a LGTM without any comment. Come on, guys... lets improve ACS, not make it worse.

In the past - not long ago - we had problems with HyperV, not even used in our environment, and also with static fields not initialised properly that caused systemVMs to not start at all. If I see a change of +4k line 380 files in a tightly-coupled system like ACS, I would test it as much as I could before saying anything.

@rafaelweingartner @pedro-martins : do you have a test environment?

Cheers,
Wilder

and later we see:

We now found a point of improvement in our CI. The point of CI is that we can rely on it and that a reviewer does not need to run tests every time, exceptions are there to confirm this rule, of course.

@wilderrodrigues I got your point, and I also hate when projects that I work start breaking as a consequence of bad/poor code. Here goes an explanation in a timeline manner, so you can get a better understanding of everything that happened:

When I assigned that “task” to an intern (@pedro-martins), he executed it going class by class literally (I have to give credits for him to be so calm and controlled, I would not stand to do that). While doing that he noted down classes that he was changing the code. When he pushed, @DaanHoogland pointed out that the PR was adding more lines then removing due to code formatting. Therefore, he created a script that was fed with that list of classes and performed the changes (deleted the lines that had to be deleted and changed the variable names that had to be changed). Another problem arose, for some reason in a few classes such as “VMwareGuru” there are static methods that do not need to be static and that use the “logger” variable, with our PR that had to be changed. He was supposed to find each of the classes that had a problem from that “ComponentLifecycleBase” hierarchic and fix it. Sadly, he missed one class out of 380. We indeed tried to build the whole project with a maven install, but it did not work at that time, there was a problem with a class that we have not touched.

After that the PR 714 was merged by @DaanHoogland, and then reverted by @karuturi.

When I noticed that problem, I got the task and created a script that really builds the whole “ComponentLifecycleBase” hierarchic and executes the deletes and changes on variable names that are needed. Then I manually check the classes that have some problem, around 8 classes that have static methods that do not need to be static. I created a new PR #778 for that, because we did not find a way to re-open the PR #714. The first time I pushed to PR 778 I successfully did a maven install on ACS to check for compilation problems that might be caused by our change. Then the time passed on (conflicts appeared) and I did another two rebases, the first one I hit a problem with a missing class on “cloud-plugin-network-vcs” and the last one another missing class on “cloud-framework-db”.

I had sent an email asking about the first missing class I found, but got no answers.

Now to answer your first question, I indeed created a script, but just after the PR 714 was reverted (we were not kidding about that). Moreover, the pushes I did to PR 778 I made sure to execute a maven run to build the project and see if everything was ok. Sadly I hit other problems with missing classes.

Sadly during PR 714, we had problems with Travis build timeouts at the same time that we were working on PR 714, if Travis CI was working properly we would have spotted the problem and the PR 714 would not need to be reverted.

@wilderrodrigues now your last point, we indeed have a production and test environments running ACS 4.3.2 (both the same version because we are extending ACS, everything we build is added into our 4.3.2, tested and then pushed to our production environment), that is why we are not building the components changed and testing it here. We did not see a reason to add this PR was not also added to 4.3.2.

"In this particular case, how is possible to get 2 LGTM without even compiling the project locally?"

I was depending on the info Daan refers to and, as such, did not build it locally.

By the time I came into the picture with regards to this PR, it was mainly a matter of looking through 361 files for changes to the name of a variable. Not sure how verbose of a comment you'd like for that, but it looked reasonable to me.

Perhaps we need to document on our Wiki exactly what needs to go into a review (including comments).

A lot of reviews for PRs that I've seen have taken my approach here: The PR submitter asked for another review, a reviewer took time to go through the code online, but didn't compile because he/she expected asfbot's SUCCESS meant it was OK from that standpoint, then wrote "LGTM."

I could say this again, but it doesn't seem like it's a popular opinion with regards to CloudStack development:

If we are really "late in the game" with regards to the 4.6 release, then probably half or more of the PRs we put into the codebase should not be going in at this point (they should be going into 4.7).

The way most software development I've been involved with over the past 17 years works is that defect and enhancement tickets are prioritized by a cross-functional team (it doesn't have to be cross functional, though, for CS).

These defect/enhancement priorities along with the stage at which we are in development (i.e. how close we are to our release deadline) inform what actually goes into the current release and what is postponed to the next release.

We have a habit with CS of just continuously churning the codebase up until the very last moment.

If we had an awesome suite of regression tests, we might be able to get away with that. I say "we might," but - in reality - it's still not a good idea to do that. Many defects don't show up until the code has "soaked" over long periods of time and we don't have any way to protect against that the way we develop and test CS code.

@mike-tutkowski we don't have an awesome suite of regression tests because we don't write them. And if so, we should not rely on the builds we have, like @DaanHoogland mentioned, because we know they are flakey at best. I do agree with you when you say that we should make the review process a bit more detailed and formalised. Simply giving out a LGTM because someone else already did and we don't see anything strange while skimming through the code, is definitely not enough.

Right, Miguel (as you and I were discussing for that other PR with regards to tests).

My point then is we especially should not be putting in this much code so close to a 4.6 GA. No commercial-grade software I've ever worked on does that (and they did have awesome suites of regression tests).

I completely agree with you in this Mike. We should stop churning in features when we don't even have a proper build that prevents syntax errors!

Maybe the compiler is overworked and just did a cursory glance and reported back "LGTM." ;)

Hi @rafaelweingartner ,

I understood that @pedro-martins went through a hard time checking class by class, but I would not discuss credits and that kind of stuff here because it could sound like an excuse. Being pragmatic: the PR is huge, lacks unit/integration tests and did not even compile. We are all here to help and understand how to get all software/system engineers to commit code that keeps us, at least, in the same quality level we have now: not decreasing our already poor coverage.

Concerning your environment: nice you have 4.3.2 in production and test. However, if you are changing master - 4k lines - it would be extremely nice to have a test environment to cover your changes. If building a new test environment would cost too much time, you could at least have something like:

• 1 VM with mysql and the source code
• Build your branch on that VM
• run the integration tests - with the simulator, at least
• if all goes fine, create the PR

component/test_routers.py
component/test_vpc_routers.py
component/test_vpc_vm_life_cycle.py
smoke/test_vm_life_cycle.py

All instructions about how to run tests and the simulator can be found here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Validating+check-ins+for+your+local+changes%2C+using+Simulator

If you want to build a DevCloud environment and use it to execute more advanced integration tests against, please check here: https://cwiki.apache.org/confluence/display/CLOUDSTACK/DevCloud

We are here to help.

Cheers,
Wilder

I will add the same comment to the opened PR instead. I believe is better to follow up overt here.