Allon Mureinik has posted comments on this change.
Change subject: core: change iso prefix command to be vds broker command
......................................................................
Patch Set 6:
(4 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java
Line 788: getVm().setVmPayload(getParameters().getVmPayload());
Line 789: }
Line 790:
Line 791: if (!vm.isAutoStartup() &&
!StringUtils.isEmpty(getVm().getIsoPath())
Line 792: &&
getIsoDomainListSyncronizer().findActiveISODomain(getVm().getStoragePoolId())
== null) {
can you explain this change?
Line 793: return
failCanDoAction(VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO);
Line 794: }
Line 795:
Line 796: return true;
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/IsoPrefixVDSCommand.java
Line 10: import
org.ovirt.engine.core.vdsbroker.irsbroker.StoragePoolInfoReturnForXmlRpc;
Line 11:
Line 12: public class IsoPrefixVDSCommand<T extends
VdsAndPoolIDVDSParametersBase> extends VdsBrokerCommand<T> {
Line 13:
Line 14: private static Map<Guid, String> storagePoolId2IsoPrefix = new
ConcurrentHashMap<Guid, String>();
map should be final
Line 15: private static ConcurrentHashMap<Guid, Object>
storagePoolId2LockObj = new ConcurrentHashMap<Guid, Object>();
Line 16:
Line 17: public IsoPrefixVDSCommand(T parameters) {
Line 18: super(parameters);
Line 33:
Line 34: storagePoolId2IsoPrefix.put(storagePoolId,
Line 35:
returnValue.mStoragePoolInfo.containsKey(IrsProperties.isoPrefix) ?
Line 36:
returnValue.mStoragePoolInfo.get(IrsProperties.isoPrefix).toString()
Line 37: : StringUtils.EMPTY);
This is quite a long "?" clause...
perhaps replace it with a good old fashioned if-else construct?
Line 38: } catch (Exception ex) {
Line 39: log.errorFormat("IsoPrefix Failed to get storage
pool info (vds {0}).", getParameters().getVdsId());
Line 40: return StringUtils.EMPTY;
Line 41: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ResetISOPathVDSCommand.java
Line 10: }
Line 11:
Line 12: @Override
Line 13: protected void executeVDSCommand() {
Line 14:
IsoPrefixVDSCommand.clearCachedIsoPrefix(getParameters().getStoragePoolId());
Not a fan of command X calling command Y in such a fashion.
Can't quite put my finger on it, but something here seems off.
Line 15: }
--
To view, visit http://gerrit.ovirt.org/17815
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I272ce7b0407bf83bd47646941630362ecf0b18cc
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
