cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alena Prokharchyk" <>
Subject Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Date Mon, 05 May 2014 18:27:41 GMT

This is an automatically generated e-mail. To reply, visit:

Alex, generic logic looks good to me. But still some things need to be fixed.

1) RegionVO

@Column(name = "active")
    private boolean active;

Explicitly set it to be active by default. Just setting it in the DB might not be enough because
DAO would always default boolean to false if not set in the constructor/intance variable declaration.
It should look like:

@Column(name = "active")

private boolean active = true;

2) Please make the description for the new parameters in the addRegion API , more descriptive

@Parameter(name = ApiConstants.MODE, type = CommandType.STRING, required = true, description
= "Region service active")
    private String mode;

3) AccountManager

* boolean updateUser(final Long id, String userName, String firstName, String lastName, String
password, String email, String apiKey, String secretKey, String timeZone, Account.State state);

Why do we pass account state to the call? there should be only user info passed in. If the
account needs to be updated, do it in the separate command; if you need to retrive the account
state - get the account object from the user object. Please remove this parameter

* Why did you change the method signature for the

UserAccount disableUser(long userId) to disableUser(User user)? The only one info that you
have to pass to the manager - is the userId. Please change it back.

The same for  boolean enableUser(User user), boolean lockAccount(Account account);

These changes don't seem to be critical or used for/by your plugin, so please change them
back to the original way of doing things. Its not a good practice to change methods defined
in the interfaces - AccountManager/AccountService - unless there is a real need for it.

* boolean updateAccount(AccountVO account, String newAccountName, String newNetworkDomain,
final Map<String, String> details, Account.State state);

As updateAccount doesn't allow account state update, please remove Account.State from the
method signature


Why did you change createDomainResponse method? 

public DomainResponse createDomainResponse(Domain domain)?

To summarize it all, I would suggest to eliminate (revert) all the changes done to account/domain
managers that your code doesn't actually need. It would make the changes list much shorter
and easier for review.

- Alena Prokharchyk

On May 4, 2014, 9:12 p.m., Alex Ough wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> -----------------------------------------------------------
> (Updated May 4, 2014, 9:12 p.m.)
> Review request for cloudstack.
> Repository: cloudstack-git
> Description
> -------
> This is the review request for the core changes related with #17790 that has only the
new plugin codes.
> Diffs
> -----
>   api/src/com/cloud/event/ 39ef710 
>   api/src/com/cloud/user/ 7e37b38 
>   api/src/org/apache/cloudstack/api/ fdb4558 
>   api/src/org/apache/cloudstack/api/ f6f21ae 
>   api/src/org/apache/cloudstack/api/ 10fb6df 
>   api/src/org/apache/cloudstack/api/command/admin/region/ f6743ba 
>   api/src/org/apache/cloudstack/api/command/admin/region/ b08cbbb

>   api/src/org/apache/cloudstack/api/response/ 6c74fa6 
>   api/src/org/apache/cloudstack/query/ 01af631 
>   api/src/org/apache/cloudstack/region/ df64e44 
>   api/src/org/apache/cloudstack/region/ afefcc7 
>   api/test/org/apache/cloudstack/api/command/test/ 10c3d85 
>   client/pom.xml d8dbde7 
>   client/tomcatconf/ 45debe4 
>   engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
>   engine/schema/src/org/apache/cloudstack/region/ 608bd2b 
>   plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/
>   plugins/pom.xml 9b391b8 
>   server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2

>   server/src/com/cloud/api/ 67e47f7 
>   server/src/com/cloud/api/ 38f2f0b 
>   server/src/com/cloud/api/dispatch/ e9bdd8b 
>   server/src/com/cloud/api/query/ 3abb944 
>   server/src/com/cloud/api/query/ d06e1d1 
>   server/src/com/cloud/event/ 28e5680 
>   server/src/com/cloud/server/ bce2930 
>   server/src/com/cloud/user/ 03bf842 
>   server/src/com/cloud/user/ 2070ee6 
>   server/src/com/cloud/user/ f72b18a 
>   server/src/com/cloud/user/ fbbe0c2 
>   server/src/org/apache/cloudstack/region/ 6f25481 
>   server/src/org/apache/cloudstack/region/ 8910714 
>   server/src/org/apache/cloudstack/region/ 98cf500 
>   server/test/com/cloud/api/dispatch/ 12051a6 
>   server/test/com/cloud/user/ f373cba 
>   server/test/com/cloud/user/ 7dddefb 
>   server/test/org/apache/cloudstack/region/ d7bc537 
>   setup/db/db/schema-440to450.sql 2bd5386 
>   ui/scripts/regions.js 66dae8c 
> Diff:
> Testing
> -------
> 1. Successfully tested real time synchronization as soon as resources are created/deleted/modified
in one region.
> 2. Successfully tested full scans to synchronize resources that were missed during real
time synchronization because of any reasons like network connection issues.
> 3. The tests were done manually and also automatically by randomly generating changes
each region.
> Thanks,
> Alex Ough

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message