-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17790/#review38761
-----------------------------------------------------------
1) You are breaking API compatiblity with introducing new required parameters
to addRegionsCmd. Please fix to make them optional.
@Parameter(name = ApiConstants.USERNAME, type = CommandType.STRING, required =
true, description = "Region service username")
55
private String username;
56
57
@Parameter(name = ApiConstants.PASSWORD, type = CommandType.STRING,
required = true, description = "Region service password")
58
private String password;
59
60
@Parameter(name = ApiConstants.MODE, type = CommandType.STRING, required =
true, description = "Region service sync mode")
61
private String mode;
2) As I said previously numerous amount of time, please move all code related
to your plugin, to the your plugin package. AccountSubscriber.java
DomainSubscirber.java
MultiRegionEventBus
MultiRegionSubscriber.java
UserSubscriber.java
AccountSubscriberTest.java
DomainSubscriberTest.java
MultiRegionSubscriberTest.java
UserSubscriberTest.java
shouldn't be placed under RabbitMQ plugin package. They should be defined in
your own plugin.
3) Please submit 2 patches as Chiradeep suggested: first - modifications for CS
Services/Apis; second - for your plugin
4) RMapDaoImpl
Please replace the use of transaction legacy code, with using new transaction
implementation:
Old implementation that has to be fixed:
@Override
51
public synchronized RmapVO create(RmapVO rmap) {
52
TransactionLegacy txn = TransactionLegacy.currentTxn();
53
try {
54
txn.start();
55
persist(rmap);
56
txn.commit();
57
return rmap;
58
} catch (Exception e) {
59
s_logger.error("Unable to create rmap due to " + e.getMessage(), e);
60
txn.rollback();
61
return null;
62
}
63
}
For new style, please refer to any CS class using transaction. For example,
updateLoginAttempts in AccountManagerImpl.
5) You've modified very important Dao classes - GenericDaoBase/SQLGenerator.
Please add unittests for that.
6) Add tests for ApiDispatcher changes please.
7) You've added new field to the UserAccountJoinVO.java, but did you modify the
DB upgrade scripts?
8)Question about AccountManagerImpl
enableAccount(long accountId, Date modified)
Why adding modified to the signature? I thought modified is somethign that
should be set in the method itself, the moment when modification is done?
9) Instead of changing the methods in AccountManager/DomainManager by adding
"removed"/"modified" field to the method signature, please introduce the new
method. Most of the times the method will be called with these values being
null. In this case, we should avoid changing existing stuff, but have to
introduce new methods. CAn you also explain the reason why these fields were
added to the method signatures.
10) Important - remove this check from the bunch of the calls in AccountManager
if (CallContext.current()
CallContext should never be null. If its null when your plugin is utilized,
then there is something wrong with that, and you have to fix your plugin to set
CallContext.
11) As I've already pointed out :) please remove the repeating occurances of
+ URLEncoder.encode(getSessionKey(), "UTF-8");
Imagine if the encoding changes at some point. You would have to go and change
it in 10000 places
12) Utilize use of @Inject and Sprint framework instead of doing things like
this please.
in public LocalAccountManager() {
44
this.domainDao = ComponentContext.getComponent(DomainDao.class);
45
this.accountManager =
ComponentContext.getComponent(AccountManager.class);
46
}
Fix all your processors to use Spring.
13) Don't concatenate stings this way:
String newDomainName = "D" + generateRandString();
String newNetworkDomain = "ND" + generateRandString();
Use StringBuilder instead.
The most important thing - your code shouldn't be a part of either regions or
event rabbit mq packages. It all has to be a part of a separate plugin. So
please create it, and move all the classes below there:
AccountSubscriber.java
DomainSubscirber.java
MultiRegionEventBus
MultiRegionSubscriber.java
UserSubscriber.java
AccountSubscriberTest.java
DomainSubscriberTest.java
MultiRegionSubscriberTest.java
UserSubscriberTest.java
Processors:
.............
DomainFullSyncProcessor.java
..............
UserService.java
And related tests
- Alena Prokharchyk
On March 26, 2014, 1:32 p.m., Alex Ough wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17790/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 1:32 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Currently, under the environment of cloudstack with multiple regions, each
> region has its own management server running with a separate database, which
> will cause data discrepancies when users create/update/delete
> domain/account/user data independently in each management server. So to
> support multiple regions and provide one point of entry for each customer,
> this implementation duplicates domain/account/user information of customers
> in one region to all of the regions independently whenever there is any
> change.
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4992
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Domain-Account-User+Sync+Up+Among+Multiple+Regions
>
>
> Diffs
> -----
>
> api/src/com/cloud/domain/Domain.java c4755d7
> api/src/com/cloud/event/EventTypes.java ec54ea1
> api/src/com/cloud/user/Account.java 99ef954
> api/src/com/cloud/user/AccountService.java a9be292
> api/src/com/cloud/user/User.java 36e9028
> api/src/com/cloud/user/UserAccount.java c5a0637
> api/src/org/apache/cloudstack/api/ApiConstants.java 14df653
> api/src/org/apache/cloudstack/api/BaseCmd.java e869ddf
> 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/AccountResponse.java b7d30ca
> api/src/org/apache/cloudstack/api/response/DomainResponse.java 0c0281e
> api/src/org/apache/cloudstack/api/response/RegionResponse.java 6c74fa6
> api/src/org/apache/cloudstack/api/response/UserResponse.java 40e1561
> 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
>
> engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
> 08efb83
> engine/schema/src/com/cloud/domain/DomainVO.java f6494b3
> engine/schema/src/com/cloud/rmap/RmapVO.java PRE-CREATION
> engine/schema/src/com/cloud/rmap/dao/RmapDao.java PRE-CREATION
> engine/schema/src/com/cloud/rmap/dao/RmapDaoImpl.java PRE-CREATION
> engine/schema/src/com/cloud/user/AccountVO.java fb1b58a
> engine/schema/src/com/cloud/user/UserAccountVO.java cef9239
> engine/schema/src/com/cloud/user/UserVO.java 68879f6
> engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b
> framework/db/src/com/cloud/utils/db/Attribute.java 82c2bdb
> framework/db/src/com/cloud/utils/db/GenericDao.java cb401cd
> framework/db/src/com/cloud/utils/db/GenericDaoBase.java cecea84
> framework/db/src/com/cloud/utils/db/SqlGenerator.java befe34b
>
> plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/AccountSubscriber.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/DomainSubscriber.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/MultiRegionEventBus.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/MultiRegionSubscriber.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/UserSubscriber.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/AccountSubscriberTest.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/DomainSubscriberTest.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/MultiRegionSubscriberTest.java
> PRE-CREATION
>
> plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/UserSubscriberTest.java
> PRE-CREATION
>
> plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
> fa7be58
> server/src/com/cloud/api/ApiDispatcher.java 5bdefe7
> server/src/com/cloud/api/ApiResponseHelper.java 81bfe21
> server/src/com/cloud/api/query/dao/AccountJoinDaoImpl.java 5e087fd
> server/src/com/cloud/api/query/dao/UserAccountJoinDaoImpl.java 923a238
> server/src/com/cloud/api/query/vo/AccountJoinVO.java 8d642ed
> server/src/com/cloud/api/query/vo/UserAccountJoinVO.java ed29284
> server/src/com/cloud/event/ActionEventUtils.java 9724d99
> server/src/com/cloud/projects/ProjectManagerImpl.java 5a0ed1c
> server/src/com/cloud/server/StatsCollector.java 548587c
> server/src/com/cloud/user/AccountManager.java 983caf1
> server/src/com/cloud/user/AccountManagerImpl.java 186cfb2
> server/src/com/cloud/user/DomainManager.java f72b18a
> server/src/com/cloud/user/DomainManagerImpl.java b2a478e
> 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/src/org/apache/cloudstack/region/multi/api/AccountInterface.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/api/BaseInterface.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/api/DomainInterface.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/api/UserInterface.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/AccountFullSyncProcessor.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/service/AccountService.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/service/BaseService.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/DomainFullSyncProcessor.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/service/DomainService.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/service/FullScanner.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/FullSyncProcessor.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/LocalAccountManager.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/LocalDomainManager.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/service/LocalUserManager.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/RemoteAccountEventProcessor.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/RemoteDomainEventProcessor.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/RemoteEventProcessor.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/RemoteUserEventProcessor.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/service/UserFullSyncProcessor.java
> PRE-CREATION
> server/src/org/apache/cloudstack/region/multi/service/UserService.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorAccountLocalGenerator.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorAccountLocalGeneratorEvent.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorAutoGenerator.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorDomainLocalGenerator.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorDomainLocalGeneratorEvent.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorLocalGenerator.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorUserLocalGenerator.java
> PRE-CREATION
>
> server/src/org/apache/cloudstack/region/multi/simulator/SimulatorUserLocalGeneratorEvent.java
> PRE-CREATION
> server/test/com/cloud/user/MockAccountManagerImpl.java 62e7fc8
> server/test/com/cloud/user/MockDomainManagerImpl.java 7dddefb
> server/test/org/apache/cloudstack/region/RegionManagerTest.java d7bc537
>
> server/test/org/apache/cloudstack/region/multi/api/AccountInterfaceTest.java
> PRE-CREATION
> server/test/org/apache/cloudstack/region/multi/api/BaseInterfaceTest.java
> PRE-CREATION
> server/test/org/apache/cloudstack/region/multi/api/DomainInterfaceTest.java
> PRE-CREATION
> server/test/org/apache/cloudstack/region/multi/api/UserInterfaceTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/service/AccountFullSyncProcessorTest.java
> PRE-CREATION
> server/test/org/apache/cloudstack/region/multi/service/BaseServiceTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/service/DomainFullSyncProcessorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/service/RemoteAccountEventProcessorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/service/RemoteDomainEventProcessorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/service/RemoteUserEventProcessorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/service/UserFullSyncProcessorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorAccountLocalGeneratorEventTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorAccountLocalGeneratorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorDomainLocalGeneratorEventTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorDomainLocalGeneratorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorLocalGeneratorTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorUserLocalGeneratorEventTest.java
> PRE-CREATION
>
> server/test/org/apache/cloudstack/region/multi/simulator/SimulatorUserLocalGeneratorTest.java
> PRE-CREATION
> setup/db/db/schema-430to440.sql acc7e21
> ui/scripts/regions.js 66dae8c
>
> Diff: https://reviews.apache.org/r/17790/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
>
>
