You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wilderrodrigues <gi...@git.apache.org> on 2015/06/30 14:41:51 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring NiciraNVP res...
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/546
CLOUDSTACK-8590 Refactoring NiciraNVP resource
Hi there,
The motivation behind the refactor of the NiciraNvpResource class was to decouple the execute of the commands, which was handled into several private methods in the class.
With the refactor, we were able to decouple all the commands by wrapping them with another class. The extra classes are declared with the ResourceWrapper annotation, which makes possible to get all the classes loaded into a map inside the implementation of the RequestWrapper class, in that case the NiciraNvpRequestWrapper.
New tests were added in order to increase test coverage. The current status is:
* Resource package: 86.7% coverage
* Utils package: 95.1% coverage
* Wrapper package: 97.5% coverage
In addition, the previous retry mechanism has been improved. Instead of having an instance variable to control the retries and the extra methods with the number of retries passed as parameter, we now have a Map which holds the commands and the number of retry. Once the number of retries reaches ZERO, the given command is removed from the Map.
An issue has been created in order to follow up with this refactor: https://issues.apache.org/jira/browse/CLOUDSTACK-8590
The changes do not affect the behaviour of the NiciraNvpResource. As a proof of that, all the existing tests worked without any change. This is a pure structural change on the code.
This PR consists of 11 commits. I tried to combine the work on the command wrappers, retry mechanism, unit tests and general stuff.
Cheers,
Wilder
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/schubergphilis/cloudstack refactor/nicira_resource
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/546.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #546
----
commit fd27b7f96b7e257e4220d072f91dafb2bbfe4fa4
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-22T13:57:15Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Adding the NiciraNvpWrapper
- This class will keep track of all Wrappers of the Nicira NVP Plugin
commit 2e0d01e6e90d100a8fc0eeebac22ef4d4f644d2c
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-23T07:20:47Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Refactoring NiciraNvpResource
- Added NiciraNvpRequestWrapper
- Removing 1 execute methods form NiciraNvpResource
- Added 1 unit test
commit 8b9b3dc8a28dba60ac42cdaef51585c097d1223f
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-23T07:38:56Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Refactoring NiciraNvpResource
- Added NiciraNvpMaintainCommandWrapper
- Removing 1 execute methods form NiciraNvpResource
- Added 1 unit test
commit bc349571e5c896f24b7fab941aa5a0832ebe0ce3
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-23T08:47:23Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Refactoring NiciraNvpResource
- Added NiciraNvpUtilities and NiciraNvpCreateLogicalSwitchCommandWrapper
- Removing 1 execute methods form NiciraNvpResource
- Added 1 unit test
commit ea7c38311cb3ef1005d5e3a1d2c6c8ebe373b10e
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T07:47:57Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Adding command wrappers NiciraNvpCreateLogicalSwitchCommandWrapper and NiciraNvpDeleteLogicalSwitchCommandWrapper
- Refactoring the retry mechanism
- Applying the new retry mechanism to current wrappers and old methods in NiciraNvpResource
- Adding 2 tests
- Fixing the testRetries() in NiciraNvpResourceTest class
commit 40a320a267d4bdfabc03a76e59f12087fbd5c776
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T08:22:25Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Adding NiciraNvpCreateLogicalSwitchPortCommandWrapper
- Removing unsued field from NiciraNvpResourceTest
commit 2a8332870d2528ffbef6a6fd3ab6f0ba3e0880d7
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T09:07:59Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Adding NiciraNvpDeleteLogicalSwitchPortCommandWrapper
commit 6f4f8a07991ab4e320842bb8c05c6d17cb7d2c06
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T09:18:58Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Adding NiciraNvpCreateLogicalRouterCommandWrapper
- Adding NiciraNvpDeleteLogicalSwitchPortCommandWrapper
- Adding NiciraNvpFindLogicalSwitchPortCommandWrapper
- Adding NiciraNvpUpdateLogicalSwitchPortCommandWrapper
- Decoupling private methods from NiciraNvpResource
commit b0dde6683b5269edb40da923893b65e88e33d9d9
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T09:34:29Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Adding remaining command wrappers
commit c9718f77060baa396ab435c37eeb168f7f0b0e7d
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T10:11:45Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Increasing test coverage
- resource package: 86.7%
- utils package: 95.1%
- wrapper package: 97.5%
commit df6c70ee57ebd59246a9436f8d9878231e173472
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-30T10:35:43Z
CLOUDSTACK-8590 - Refactoring NiciraNVP resource
- Change the retry() method to return Answer.createUnsupportedCommandAnswer(command) instead of throwing an exception
----
---
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.
---
Re: [GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring
NiciraNVP res...
Posted by Wilder Rodrigues <WR...@schubergphilis.com>.
Hey guys,
Anyone willing to have a look at this PR?
Thanks in advance.
Cheers,
Wilder
Sent from my iPhone
> On 30 Jun 2015, at 14:41, wilderrodrigues <gi...@git.apache.org> wrote:
>
> GitHub user wilderrodrigues opened a pull request:
>
> https://github.com/apache/cloudstack/pull/546
>
> CLOUDSTACK-8590 Refactoring NiciraNVP resource
>
> Hi there,
>
> The motivation behind the refactor of the NiciraNvpResource class was to decouple the execute of the commands, which was handled into several private methods in the class.
>
> With the refactor, we were able to decouple all the commands by wrapping them with another class. The extra classes are declared with the ResourceWrapper annotation, which makes possible to get all the classes loaded into a map inside the implementation of the RequestWrapper class, in that case the NiciraNvpRequestWrapper.
>
> New tests were added in order to increase test coverage. The current status is:
>
> * Resource package: 86.7% coverage
> * Utils package: 95.1% coverage
> * Wrapper package: 97.5% coverage
>
> In addition, the previous retry mechanism has been improved. Instead of having an instance variable to control the retries and the extra methods with the number of retries passed as parameter, we now have a Map which holds the commands and the number of retry. Once the number of retries reaches ZERO, the given command is removed from the Map.
>
> An issue has been created in order to follow up with this refactor: https://issues.apache.org/jira/browse/CLOUDSTACK-8590
>
> The changes do not affect the behaviour of the NiciraNvpResource. As a proof of that, all the existing tests worked without any change. This is a pure structural change on the code.
>
> This PR consists of 11 commits. I tried to combine the work on the command wrappers, retry mechanism, unit tests and general stuff.
>
> Cheers,
> Wilder
>
> You can merge this pull request into a Git repository by running:
>
> $ git pull https://github.com/schubergphilis/cloudstack refactor/nicira_resource
>
> Alternatively you can review and apply these changes as the patch at:
>
> https://github.com/apache/cloudstack/pull/546.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
> This closes #546
>
> ----
> commit fd27b7f96b7e257e4220d072f91dafb2bbfe4fa4
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-22T13:57:15Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Adding the NiciraNvpWrapper
> - This class will keep track of all Wrappers of the Nicira NVP Plugin
>
> commit 2e0d01e6e90d100a8fc0eeebac22ef4d4f644d2c
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-23T07:20:47Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Refactoring NiciraNvpResource
> - Added NiciraNvpRequestWrapper
> - Removing 1 execute methods form NiciraNvpResource
> - Added 1 unit test
>
> commit 8b9b3dc8a28dba60ac42cdaef51585c097d1223f
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-23T07:38:56Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Refactoring NiciraNvpResource
> - Added NiciraNvpMaintainCommandWrapper
> - Removing 1 execute methods form NiciraNvpResource
> - Added 1 unit test
>
> commit bc349571e5c896f24b7fab941aa5a0832ebe0ce3
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-23T08:47:23Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Refactoring NiciraNvpResource
> - Added NiciraNvpUtilities and NiciraNvpCreateLogicalSwitchCommandWrapper
> - Removing 1 execute methods form NiciraNvpResource
> - Added 1 unit test
>
> commit ea7c38311cb3ef1005d5e3a1d2c6c8ebe373b10e
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T07:47:57Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Adding command wrappers NiciraNvpCreateLogicalSwitchCommandWrapper and NiciraNvpDeleteLogicalSwitchCommandWrapper
> - Refactoring the retry mechanism
> - Applying the new retry mechanism to current wrappers and old methods in NiciraNvpResource
> - Adding 2 tests
> - Fixing the testRetries() in NiciraNvpResourceTest class
>
> commit 40a320a267d4bdfabc03a76e59f12087fbd5c776
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T08:22:25Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Adding NiciraNvpCreateLogicalSwitchPortCommandWrapper
> - Removing unsued field from NiciraNvpResourceTest
>
> commit 2a8332870d2528ffbef6a6fd3ab6f0ba3e0880d7
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T09:07:59Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Adding NiciraNvpDeleteLogicalSwitchPortCommandWrapper
>
> commit 6f4f8a07991ab4e320842bb8c05c6d17cb7d2c06
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T09:18:58Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Adding NiciraNvpCreateLogicalRouterCommandWrapper
> - Adding NiciraNvpDeleteLogicalSwitchPortCommandWrapper
> - Adding NiciraNvpFindLogicalSwitchPortCommandWrapper
> - Adding NiciraNvpUpdateLogicalSwitchPortCommandWrapper
> - Decoupling private methods from NiciraNvpResource
>
> commit b0dde6683b5269edb40da923893b65e88e33d9d9
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T09:34:29Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Adding remaining command wrappers
>
> commit c9718f77060baa396ab435c37eeb168f7f0b0e7d
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T10:11:45Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Increasing test coverage
> - resource package: 86.7%
> - utils package: 95.1%
> - wrapper package: 97.5%
>
> commit df6c70ee57ebd59246a9436f8d9878231e173472
> Author: wilderrodrigues <wr...@schubergphilis.com>
> Date: 2015-06-30T10:35:43Z
>
> CLOUDSTACK-8590 - Refactoring NiciraNVP resource
>
> - Change the retry() method to return Answer.createUnsupportedCommandAnswer(command) instead of throwing an exception
>
> ----
>
>
> ---
> 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.
> ---
[GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring NiciraNVP res...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/546#issuecomment-118011859
@remibergsma and @karuturi
You are right.
I believe both @bhaisaab and I forgot that. Once he said "go ahead with the merge" I just merged the thing.
Apologies.
Cheers,
Wilder
---
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.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring NiciraNVP res...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/546
---
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.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring NiciraNVP res...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/546#issuecomment-117653714
@wilderrodrigues LGTM, and Travis is green though there is no way for me to test it against the real h/w. Please go ahead with the merge.
---
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.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring NiciraNVP res...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/546#issuecomment-117896507
shouldnt we wait for another review?
---
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.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8590 Refactoring NiciraNVP res...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/546#issuecomment-118009819
@karuturi I agree, we should only proceed after two LGTMs.
Let met give the second LGTM, after the fact.
---
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.
---