cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alena Prokharchyk" <alena.prokharc...@citrix.com>
Subject Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Date Mon, 05 May 2014 21:22:17 GMT


> On May 5, 2014, 6:27 p.m., Alena Prokharchyk wrote:
> > 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")
> > 55  
> >     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
> > 
> > 
> > 4) ApiResponseHelper.java
> > 
> > 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.
> 
> Alex Ough wrote:
>     for 3) AccountManager
>     
>     * updateUser in 'state' : it is necessary during the Full Sync when an object's information
is not same with one in other regions.
>     * new signatures : they are necessary during the Full Sync not to generate events
while changing them. They are just another methods to manage objects, so can you show me why
they are not permitted?
>
> 
> Alena Prokharchyk wrote:
>     Alex, correct me if I'm wrong. In most of your code you use CS APIs to retrieve/update/delete/disable(disable
changes the state) the CS objects (user/domain/account - updateAccount/updateDomain/deleteAccount
etc). And you do it in each reason, and that generates the events for all the regions when
these APIs are processed. Like for example, I see these calls:
>     
>       public JSONObject updateAccount(String accountId, String currentName, String newName,
String details, String domainId, String networkDomain) throws APIFailureException {
>     139	
>     140	
>             StringBuilder param = buildCmd("updateAccount");
>     141	
>             param.append(append(ApiConstants.ID, accountId));
>     142	
>             if (currentName != null) param.append(append(ApiConstants.ACCOUNT, currentName));
>     143	
>             if (newName != null) param.append(append(ApiConstants.NEW_NAME, newName));
>     144	
>             if (details != null) param.append(append(ApiConstants.ACCOUNT_DETAILS, details));
>     145	
>             if (domainId != null) param.append(append(ApiConstants.DOMAIN_ID, domainId));
>     146	
>             if (networkDomain != null) param.append(append(ApiConstants.NETWORK_DOMAIN,
networkDomain));
>     147	
>             param.append(appendResType("json"));
>     148	
>             param.append(appendSessionKey(encodeSessionKey()));
>     149	
>     150	
>             return sendApacheGet(param.toString());
>     151	
>         }
>     152	
>     153	
>         public JSONObject disableAccount(String accountId) throws APIFailureException
{
>     164	
>     165	
>             StringBuilder param = buildCmd("disableAccount");
>     166	
>             param.append(append(ApiConstants.ID, accountId));
>     167	
>             param.append(append(ApiConstants.LOCK, "false"));
>     168	
>             param.append(appendResType("json"));
>     169	
>             param.append(appendSessionKey(encodeSessionKey()));
>     170	
>     171	
>             return sendApacheGet(param.toString());
>     172	
>         }
>     
>     
>     
>     But in some parts of your code  you call to the AccountManager/domainManager interfaces
directly, omitting APIs, to eliminate the events generation - like for the state change? Can
you please elaborate when your code uses one model vs another? What are the rules to follow?
I would rather see things being consistent unless there is a valid reason to do otherwise.
> 
> Alex Ough wrote:
>     As I emphasized, there are 2 cases of synchronization.
>     1. real time : It just calls API interface to other regions as soon as a resource
in the local region is created/updated/removed, which is using events.
>     2. full scan : It is to cover the cases when the real time sync has been failed with
any reason like network failures or etc, which results in resource mismatches among regions.
To cover this, the local region regularly compares the corresponding resources in each remote
region and if there is any mismatch, it creates/updates/removes the resource in local region
whose last modified timestamp is older than the one of the resource in the remote region.
That's why I need a method to manage the resources without event generation.
>     
>     Let me know if you need more information.

Alex, One more clarification is needed - do you use in memory bus, or Rabbit MQ bus shared
across multiple management servers? From what you've said in #1 (It just calls API interface
to other regions as soon as a resource in the local region is created/updated/removed, which
is using events.), I assume its in-memory bus.

Then if the local region listens only to local events (assuming you use in-memory bus), and
then sends API calls to the remote regions, why can't it call the API against itself in #2?
Because from what you've said above, only local region is going to listen o local events,
and publishing them won't affect other regions as they also listen to their local events.


- Alena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20099/#review42175
-----------------------------------------------------------


On May 4, 2014, 9:12 p.m., Alex Ough wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20099/
> -----------------------------------------------------------
> 
> (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/EventTypes.java 39ef710 
>   api/src/com/cloud/user/AccountService.java 7e37b38 
>   api/src/org/apache/cloudstack/api/ApiConstants.java fdb4558 
>   api/src/org/apache/cloudstack/api/BaseCmd.java f6f21ae 
>   api/src/org/apache/cloudstack/api/ResponseGenerator.java 10fb6df 
>   api/src/org/apache/cloudstack/api/command/admin/region/AddRegionCmd.java f6743ba 
>   api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionCmd.java b08cbbb

>   api/src/org/apache/cloudstack/api/response/RegionResponse.java 6c74fa6 
>   api/src/org/apache/cloudstack/query/QueryService.java 01af631 
>   api/src/org/apache/cloudstack/region/Region.java df64e44 
>   api/src/org/apache/cloudstack/region/RegionService.java afefcc7 
>   api/test/org/apache/cloudstack/api/command/test/RegionCmdTest.java 10c3d85 
>   client/pom.xml d8dbde7 
>   client/tomcatconf/commands.properties.in 45debe4 
>   engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
489b37d 
>   engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b 
>   plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
957f708 
>   plugins/pom.xml 9b391b8 
>   server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2

>   server/src/com/cloud/api/ApiDBUtils.java 67e47f7 
>   server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b 
>   server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b 
>   server/src/com/cloud/api/query/QueryManagerImpl.java 3abb944 
>   server/src/com/cloud/api/query/ViewResponseHelper.java d06e1d1 
>   server/src/com/cloud/event/ActionEventUtils.java 28e5680 
>   server/src/com/cloud/server/ManagementServerImpl.java bce2930 
>   server/src/com/cloud/user/AccountManager.java 03bf842 
>   server/src/com/cloud/user/AccountManagerImpl.java 2070ee6 
>   server/src/com/cloud/user/DomainManager.java f72b18a 
>   server/src/com/cloud/user/DomainManagerImpl.java fbbe0c2 
>   server/src/org/apache/cloudstack/region/RegionManager.java 6f25481 
>   server/src/org/apache/cloudstack/region/RegionManagerImpl.java 8910714 
>   server/src/org/apache/cloudstack/region/RegionServiceImpl.java 98cf500 
>   server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java 12051a6 
>   server/test/com/cloud/user/MockAccountManagerImpl.java f373cba 
>   server/test/com/cloud/user/MockDomainManagerImpl.java 7dddefb 
>   server/test/org/apache/cloudstack/region/RegionManagerTest.java d7bc537 
>   setup/db/db/schema-440to450.sql 2bd5386 
>   ui/scripts/regions.js 66dae8c 
> 
> Diff: https://reviews.apache.org/r/20099/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
> 
>


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