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