> 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
>
>
