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