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