CLOUDSTACK-9315: Removed unused Classes by GabrielBrascher · Pull Request #1448 · apache/cloudstack

GabrielBrascher · 2016-03-20T08:47:49Z

Remove unused Classes:

com.cloud.agent.api.CheckStateAnswer
com.cloud.agent.api.StartupVMMAgentCommand
com.cloud.agent.api.routing.UserDataCommand
- remove from description at
  com.cloud.configuration.Config.ExecuteInSequenceNetworkElementCommands
  enum
com.cloud.agent.api.storage.UpgradeDiskCommand
com.cloud.agent.api.storage.CreatePrivateTemplateCommand
com.cloud.agent.api.storage.DestroyAnswer
- Note: there is just a line of //FIXME: Should have an DestroyAnswer at
  com.cloud.storage.resource.StoragePoolResource
com.cloud.agent.api.storage.UpgradeDiskAnswer
com.cloud.agent.api.storage.ManageVolumeAvailabilityAnswer
com.cloud.agent.api.storage.ManageVolumeAvailabilityCommand
com.cloud.exception.UsageServerException
com.cloud.info.SecStorageVmLoadInfo
com.cloud.serializer.SerializerHelper

DaanHoogland · 2016-03-21T10:05:30Z

@GabrielBrascher removing unused code is good; less is more. These are API calls and related stuff however. A large test set should be run and I am surprised no test code would be touched by this change.

GabrielBrascher · 2016-03-21T13:19:51Z

@DaanHoogland the lack of test in some classes indeed is a problem. However, these classes that I removed are not being used.

I used UCDetector (http://www.ucdetector.org/) as a plugin for Eclipse. With this tool, I discovered a lot of code without any reference (variables, methods and classes).

This pull request had the goal of removing some of these classes. To ensure that I wasn't missing anything I searched for any file that could reference some answer or API command. As I haven't found any way of these classes being used, I removed them.

Any of those API commands that I deleted are not using org.apache.cloudstack.api.APICommand (with @APICommand). Therefore, those commands are not exposed in the ACS API.

DaanHoogland · 2016-03-21T13:25:24Z

Ok, that is somewhat comforting, however still a large integration test suite should be run on this.

swill · 2016-03-21T22:07:42Z

With these major stripping out of unused code, we need to be very careful that we do not introduce integration test issues. Can you please validate that the integration test output does not change based on this code being removed?

GabrielBrascher · 2016-03-21T22:42:28Z

@DaanHoogland @swill thank you for the concern. I agree with the need for executing integration/functional tests to keep the code stable. Unfortunately, our test environment is not set yet and it may take a while. As soon as we have it operational I will run the test.

swill · 2016-03-21T22:46:48Z

Thank you @GabrielBrascher. I am also getting my CI setup so I can get tests running against these PRs, so I should be able to start validating this stuff as well. Thanks for the continued effort. :)

…asses2 Remove classes with no referencesI used UCDetector (http://www.ucdetector.org/) as a plugin for Eclipse. With this tool, I discovered a lot of code without any reference (variables, methods and classes). Following the work that was done at [#1448]; this pull request had the goal of removing some of these classes. To check if I wasn't missing anything I searched for any file that could reference some of those classes. As I haven't found any way of these classes being used, they were removed. Note that some of them I found other references, but references such as commented lines or tests, nothing that could indicate their use (as XML files configuring beans or another class instantiating an object with "new"). Waiting for tests. Please tell me if I am missing something. Removed Classes: - org.apache.cloudstack.framework.jobs.JobCancellationException (**Note:** removed variable JobCancellationException in com.cloud.utils.SerialVersionUID) - org.apache.cloudstack.ldap.NoSuchLdapUserException (**Note:** removed test file /cloud-plugin-user-authenticator-ldap/test/groovy/org/apache/cloudstack/ldap/NoSuchLdapUserExceptionSpec.groovy) - com.cloud.agent.api.storage.CreateVolumeOVAAnswer - com.cloud.exception.MissingParameterValueException - org.apache.cloudstack.api.response.StatusResponse - org.apache.cloudstack.api.response.VolumeDetailResponse - org.apache.cloudstack.api.response.UpgradeVmResponse - org.apache.cloudstack.api.response.AddIpToVmNicResponse - org.apache.cloudstack.api.response.TemplateZoneResponse (**Note:** at org.apache.cloudstack.api.response.TemplateResponse, there is this comment "To avoid breaking backwards compatibility, we still treat a template at different zones as different templates, so not embedding template_zone information in this TemplateZoneResponse set. `private Set<TemplateZoneResponse> zones;`" but right now it is not used) - org.apache.cloudstack.api.response.NicDetailResponse * pr/1453: Removed classes with no reference Signed-off-by: Will Stevens <williamstevens@gmail.com>

bvbharatk · 2016-06-07T20:42:22Z

ACS CI BVT Run

Sumarry:
Build Number 155
Hypervisor xenserver
NetworkType Advanced
Passed=68
Failed=5
Skipped=3

Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

test_vpc_vpn.py
- ContextSuite context=TestRVPCSite2SiteVpn>:setup Failing since 14 runs
- ContextSuite context=TestVpcRemoteAccessVpn>:setup Failing since 14 runs
- ContextSuite context=TestVpcSite2SiteVpn>:setup Failing since 14 runs
test_volumes.py
- test_06_download_detached_volume Failed
test_vm_life_cycle.py
- test_10_attachAndDetach_iso Failed

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_disk_offerings.py

yadvr · 2017-07-27T11:47:46Z

@GabrielBrascher can you rebase against latest master

GabrielBrascher · 2017-08-04T04:11:04Z

Rebased against latest master. Thanks @rhtyd.

yadvr · 2017-08-31T08:13:51Z

@blueorangutan package

blueorangutan · 2017-08-31T08:14:07Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-08-31T10:03:13Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1063

GabrielBrascher · 2017-09-26T20:05:21Z

@rhtyd can you please run integration tests? Thanks ;)

yadvr · 2017-09-27T04:12:20Z

@blueorangutan package

blueorangutan · 2017-09-27T04:12:34Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-09-27T04:38:17Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1112

yadvr · 2017-09-27T06:37:09Z

server/src/com/cloud/configuration/Config.java

-            "If set to true, DhcpEntryCommand, SavePasswordCommand, UserDataCommand, VmDataCommand will be synchronized on the agent side."
-                + " If set to false, these commands become asynchronous. Default value is false.",
-            null),
+                    "If set to true, DhcpEntryCommand, SavePasswordCommand, VmDataCommand will be synchronized on the agent side."


@GabrielBrascher can you fix indentations changes in this class/file?

Fixed, thanks @rhtyd.

yadvr · 2017-09-27T06:37:23Z

@blueorangutan test

blueorangutan · 2017-09-27T06:37:34Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2017-12-19T06:58:18Z

@blueorangutan package

blueorangutan · 2017-12-19T06:58:33Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-12-19T07:36:36Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1413

rafaelweingartner · 2018-10-16T18:48:26Z

@blueorangutan package

@rhtyd I took the liberty of getting this PR and fixing the conflicts. There were quite a few unused classes that we removed.

blueorangutan · 2018-10-16T18:50:02Z

@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-10-16T19:24:26Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2351

nvazquez · 2018-10-17T00:51:37Z

server/src/main/java/com/cloud/configuration/Config.java

            Boolean.class,
            "execute.in.sequence.network.element.commands",
            "false",
-            "If set to true, DhcpEntryCommand, SavePasswordCommand, UserDataCommand, VmDataCommand will be synchronized on the agent side."


Also found some reference on the schema-410to420.sql file

Good catch. I will fix that, and create a script to update the database.

nvazquez

LGTM. Verified that classes are unused, just left some minor comment

nvazquez · 2018-10-17T00:56:11Z

@blueorangutan test

blueorangutan · 2018-10-17T00:57:04Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

…mmands' param Update description of 'execute.in.sequence.network.element.commands'parameter to reflect an unused command that has been removed. The removed class command is 'UserDataCommand'.

rafaelweingartner · 2018-10-18T01:23:46Z

@nvazquez do you know what happened with the tests?

yadvr · 2018-10-18T05:42:44Z

@blueorangutan package

blueorangutan · 2018-10-18T05:43:08Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-10-18T06:22:33Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2354

nvazquez · 2018-10-18T12:19:45Z

@rafaelweingartner was a failure downloading dependencies before environment has been built, let me start them again
@blueorangutan test

blueorangutan · 2018-10-18T12:21:10Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-10-18T19:05:34Z

Trillian test result (tid-3085)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 22147 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1448-t3085-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 67 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	22.09	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	469.24	test_vpc_redundant.py

rafaelweingartner · 2018-10-28T12:59:10Z

@rhtyd can I consider these tests results as successful?

yadvr · 2018-10-28T21:24:03Z

engine/schema/src/main/resources/META-INF/db/schema-41120to41200.sql

+UPDATE `cloud`.`async_job` SET `removed` = now() WHERE `removed` IS NULL;
+
+-- PR#1448 update description of 'execute.in.sequence.network.element.commands' parameter to reflect an unused command that has been removed. The removed class command is 'UserDataCommand'.
+update configuration set description = 'If set to true, DhcpEntryCommand, SavePasswordCommand, VmDataCommand will be synchronized on the agent side. If set to false, these commands become asynchronous. Default value is false.' where name = 'execute.in.sequence.network.element.commands'; 


Can you change this to cloud configuration?

I already did it (https://github.com/apache/cloudstack/pull/1448/files#diff-69537f9fb58dcc5d2b95bb427e8e47e5). I am only creating this script to update that values for environments that already have this entry in the database.

@rafaelweingartner not the comment is to rewrite the update statement as:

UPDATE `cloud`.`configuration` SET ...;

Ah, sure the schema. Done!

yadvr · 2018-10-28T21:26:07Z

Yes @rafaelweingartner test lgtm, the failure are introduced by something else on master. I've left a minor note.

GabrielBrascher changed the title ~~Removed unused Classes~~ CLOUDSTACK-9315: Removed unused Classes Mar 20, 2016

GabrielBrascher mentioned this pull request Mar 26, 2016

Remove classes with no references #1453

Merged

GabrielBrascher force-pushed the brascher-removeUnusedClasses branch from ce405b0 to 1df846e Compare August 4, 2017 04:06

yadvr reviewed Sep 27, 2017

View reviewed changes

GabrielBrascher force-pushed the brascher-removeUnusedClasses branch 3 times, most recently from 25e0c22 to b0e0cba Compare September 27, 2017 11:29

GabrielBrascher force-pushed the brascher-removeUnusedClasses branch from b0e0cba to bfa9be6 Compare October 4, 2017 14:15

rafaelweingartner self-assigned this Oct 16, 2018

rafaelweingartner added the type:cleanup label Oct 16, 2018

rafaelweingartner added this to the 4.12.0.0 milestone Oct 16, 2018

rafaelweingartner requested a review from nvazquez October 16, 2018 18:49

rafaelweingartner requested review from DaanHoogland, marcaurele and pdion891 October 16, 2018 18:50

nvazquez reviewed Oct 17, 2018

View reviewed changes

nvazquez approved these changes Oct 17, 2018

View reviewed changes

PR#1448 update description of 'execute.in.sequence.network.element.co…

b65d820

…mmands' param Update description of 'execute.in.sequence.network.element.commands'parameter to reflect an unused command that has been removed. The removed class command is 'UserDataCommand'.

rafaelweingartner force-pushed the brascher-removeUnusedClasses branch from 28093db to b65d820 Compare October 17, 2018 12:16

rafaelweingartner approved these changes Oct 28, 2018

View reviewed changes

yadvr reviewed Oct 28, 2018

View reviewed changes

Add cloud schema to update SQL

9a1e2d5

yadvr approved these changes Oct 29, 2018

View reviewed changes

rafaelweingartner merged commit cdc6e6e into apache:master Oct 29, 2018

Conversation

GabrielBrascher commented Mar 20, 2016

Uh oh!

DaanHoogland commented Mar 21, 2016

Uh oh!

GabrielBrascher commented Mar 21, 2016

Uh oh!

DaanHoogland commented Mar 21, 2016

Uh oh!

swill commented Mar 21, 2016

Uh oh!

GabrielBrascher commented Mar 21, 2016

Uh oh!

swill commented Mar 21, 2016

Uh oh!

bvbharatk commented Jun 7, 2016

ACS CI BVT Run

Uh oh!

yadvr commented Jul 27, 2017

Uh oh!

GabrielBrascher commented Aug 4, 2017

Uh oh!

yadvr commented Aug 31, 2017

Uh oh!

blueorangutan commented Aug 31, 2017

Uh oh!

blueorangutan commented Aug 31, 2017

Uh oh!

GabrielBrascher commented Sep 26, 2017

Uh oh!

yadvr commented Sep 27, 2017

Uh oh!

blueorangutan commented Sep 27, 2017

Uh oh!

blueorangutan commented Sep 27, 2017

Uh oh!

yadvr Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 27, 2017

Uh oh!

blueorangutan commented Sep 27, 2017

Uh oh!

yadvr commented Dec 19, 2017

Uh oh!

blueorangutan commented Dec 19, 2017

Uh oh!

blueorangutan commented Dec 19, 2017

Uh oh!

rafaelweingartner commented Oct 16, 2018

Uh oh!

blueorangutan commented Oct 16, 2018

Uh oh!

blueorangutan commented Oct 16, 2018

Uh oh!

nvazquez Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Oct 17, 2018

Uh oh!

blueorangutan commented Oct 17, 2018

Uh oh!

rafaelweingartner commented Oct 18, 2018

Uh oh!

yadvr commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

blueorangutan commented Oct 18, 2018

Uh oh!

nvazquez commented Oct 18, 2018

rafaelweingartner Oct 28, 2018 •

edited

Loading

yadvr Oct 29, 2018 •

edited

Loading