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