[CLOUDSTACK-10196] Remove ejb-api 3.0 dependency by rafaelweingartner · Pull Request #2348 · apache/cloudstack

rafaelweingartner · 2017-11-30T18:20:21Z

This PR is proposing the removal of "ejb-api" 3.0 JAR. We use spring to manage objects creation and injection. Thus, spring beans do not need the annotation @Local. The @component or the bean declaration in the XML is enough to wire the object in Spring life-cycle.

marcaurele

Good cleanup - The @Component annotation is not enough, otherwise the custom spring context in CS does not know in which context to put the bean since there is a context tree structure to avoid overrides by plugins (https://cwiki.apache.org/confluence/display/CLOUDSTACK/Plug-ins%2C+Modules%2C+and+Extensions).

rafaelweingartner · 2017-12-02T20:32:49Z

Ah yeah, actually that depends.. that is why I mentioned that @component or XML declaration is enough. @Local is from a whole different framework that may be seen as an alternative to Spring, and that we do not use.

@marcaurele thanks for the review!

yadvr · 2017-12-11T16:31:34Z

Good cleanup, we'll need some extended testing on this one.

yadvr · 2017-12-15T12:35:43Z

@rafaelweingartner can you fix the conflicts?

rafaelweingartner · 2017-12-15T13:20:17Z

@rhtyd done!

yadvr · 2017-12-15T13:24:54Z

@blueorangutan package

blueorangutan · 2017-12-15T13:25:32Z

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

blueorangutan · 2017-12-15T14:11:15Z

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

yadvr · 2017-12-15T16:20:30Z

@blueorangutan package

blueorangutan · 2017-12-15T16:21:32Z

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

blueorangutan · 2017-12-15T16:43:59Z

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

yadvr · 2017-12-15T16:44:22Z

@blueorangutan test

blueorangutan · 2017-12-15T16:44:32Z

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

blueorangutan · 2017-12-16T01:09:41Z

Trillian test result (tid-1794)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30045 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2348-t1794-kvm-centos7.zip
Test completed. Failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_vpc_privategw_acl	`Failure`	56.27	test_privategw_acl.py
test_02_vpc_privategw_static_routes	`Failure`	222.51	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	127.16	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	302.84	test_privategw_acl.py
test_02_create_template_with_checksum_sha1	`Error`	5.15	test_templates.py
test_03_create_template_with_checksum_sha256	`Error`	5.14	test_templates.py
test_04_create_template_with_checksum_md5	`Error`	5.13	test_templates.py
test_01_vpc_remote_access_vpn	`Failure`	60.62	test_vpc_vpn.py

yadvr · 2017-12-16T04:07:57Z

LGTM, test LGTM as well. The failures are known issues.

yadvr · 2017-12-16T04:12:34Z

                }
            });
-
-            for (Long vmId : affectedVms) {


@rafaelweingartner why have you removed this?

Because it was not making much sense to me; for every VM ID in affectedVms it executes an asynchronous task _executorPool.schedule(new WorkerThread(), delayMs, TimeUnit.MILLISECONDS);. The odd part is that the vmId is not used. All of the tasks are scheduled to be executed are the same.

Looking at com.cloud.network.security.SecurityGroupManagerImpl.work() closely, it seems that it is indeed used for something. I was expecting it to be a generic task where we send something to be computed on.

So, there this idea of security group work, and the guys that implemented this, probably wanted to execute/process a number of work that is equal to the number of affectedVms. It would have been clear if done in some other way… I will not refactor this.
I will re-added this bit again.

@rhtyd thanks for spotting that!

yadvr · 2017-12-16T04:13:52Z

        }
    }

-    private void processScheduledWork() {


The usage of this method is commented, should we remove that too or keep this?

For me, code commented, dead codes, or code not used should always be deleted.

Alright, see line 211 in CleanupThread, maybe remove the commented method as well?

yadvr · 2017-12-16T04:15:57Z

@rafaelweingartner can you address outstanding queries, following which we can merge this.

rafaelweingartner · 2017-12-16T09:47:21Z

@rhtyd I replied your inquiries.
Can you take a look at them?

Thanks for the feedback here!

P.S. I am not able to squash the commits now. I will do that on Monday (maybe tomorrow in the evening with a bit of luck)

yadvr · 2017-12-17T09:09:17Z

@rafaelweingartner thanks, no need to squash yourself. I'll squash merge using Github. I'll re-review the changes, and kick another round of tests.

yadvr · 2017-12-17T09:13:27Z

LGTM
@blueorangutan package

blueorangutan · 2017-12-17T09:13:32Z

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

blueorangutan · 2017-12-17T09:35:13Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1389

yadvr · 2017-12-17T11:21:39Z

@blueorangutan test

blueorangutan · 2017-12-17T11:22:32Z

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

blueorangutan · 2017-12-17T19:32:26Z

Trillian test result (tid-1802)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29391 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2348-t1802-kvm-centos7.zip
Test completed. Failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_01_vpc_privategw_acl	`Failure`	56.21	test_privategw_acl.py
test_02_vpc_privategw_static_routes	`Failure`	187.13	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	121.88	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	207.22	test_privategw_acl.py
test_02_create_template_with_checksum_sha1	`Error`	5.15	test_templates.py
test_03_create_template_with_checksum_sha256	`Error`	5.14	test_templates.py
test_04_create_template_with_checksum_md5	`Error`	5.14	test_templates.py
test_01_vpc_remote_access_vpn	`Failure`	65.64	test_vpc_vpn.py

rafaelweingartner · 2017-12-17T20:22:13Z

@rhtyd commits squashed.
Thanks!

yadvr · 2017-12-18T18:20:51Z

@rafaelweingartner do we have a jira ID for this?

rafaelweingartner · 2017-12-18T18:21:31Z

Hmm, no...
I will create one now

I also Fixed QuotaAlertManagerImplTest, which was injecting mock objects manually

rafaelweingartner · 2017-12-18T18:26:34Z

@rhtyd done

yadvr · 2017-12-18T18:29:29Z

Merged based on review and test results.

rafaelweingartner force-pushed the removeEjbDependency branch from 414803d to 48838e9 Compare November 30, 2017 22:46

marcaurele approved these changes Dec 1, 2017

View reviewed changes

yadvr added this to the 4.11 milestone Dec 10, 2017

rafaelweingartner force-pushed the removeEjbDependency branch from d2541d1 to 539edb3 Compare December 15, 2017 13:17

rafaelweingartner force-pushed the removeEjbDependency branch 4 times, most recently from b28e519 to 36cb848 Compare December 15, 2017 14:43

yadvr reviewed Dec 16, 2017

View reviewed changes

rafaelweingartner force-pushed the removeEjbDependency branch from f8ddc4b to 72a0466 Compare December 17, 2017 20:22

rafaelweingartner force-pushed the removeEjbDependency branch from 72a0466 to 5f9cdf8 Compare December 18, 2017 15:48

rafaelweingartner changed the title ~~Remove ejb-api 3.0 dependency~~ [CLOUDSTACK-10196] Remove ejb-api 3.0 dependency Dec 18, 2017

[CLOUDSTACK-10196] Remove ejb-api 3.0 dependency

8b35da6

I also Fixed QuotaAlertManagerImplTest, which was injecting mock objects manually

rafaelweingartner force-pushed the removeEjbDependency branch from 5f9cdf8 to 8b35da6 Compare December 18, 2017 18:26

yadvr merged commit 3c6df7c into apache:master Dec 18, 2017

Conversation

rafaelweingartner commented Nov 30, 2017

Uh oh!

marcaurele left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Dec 2, 2017

Uh oh!

yadvr commented Dec 11, 2017

Uh oh!

yadvr commented Dec 15, 2017

Uh oh!

rafaelweingartner commented Dec 15, 2017

Uh oh!

yadvr commented Dec 15, 2017

Uh oh!

blueorangutan commented Dec 15, 2017

Uh oh!

blueorangutan commented Dec 15, 2017

Uh oh!

yadvr commented Dec 15, 2017

Uh oh!

blueorangutan commented Dec 15, 2017

Uh oh!

blueorangutan commented Dec 15, 2017

Uh oh!

yadvr commented Dec 15, 2017

Uh oh!

blueorangutan commented Dec 15, 2017

Uh oh!

blueorangutan commented Dec 16, 2017

Uh oh!

yadvr commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

yadvr Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

yadvr Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

yadvr commented Dec 16, 2017

Uh oh!

rafaelweingartner commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Dec 17, 2017

Uh oh!

yadvr commented Dec 17, 2017

Uh oh!

blueorangutan commented Dec 17, 2017

Uh oh!

blueorangutan commented Dec 17, 2017

Uh oh!

yadvr commented Dec 17, 2017

Uh oh!

blueorangutan commented Dec 17, 2017

Uh oh!

blueorangutan commented Dec 17, 2017

Uh oh!

rafaelweingartner commented Dec 17, 2017

Uh oh!

yadvr commented Dec 18, 2017

Uh oh!

rafaelweingartner commented Dec 18, 2017

Uh oh!

rafaelweingartner commented Dec 18, 2017

Uh oh!

yadvr commented Dec 18, 2017

Uh oh!

Reviewers

yadvr commented Dec 16, 2017 •

edited

Loading

rafaelweingartner commented Dec 16, 2017 •

edited

Loading