Arik Hadas has posted comments on this change.
Change subject: core: change iso prefix command to be vds broker command
......................................................................
Patch Set 8:
(3 comments)
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/IsoPrefixVDSCommand.java
Line 23: }
Line 24:
Line 25: private String getIsoPrefix() {
Line 26: Guid storagePoolId = getParameters().getStoragePoolId();
Line 27: if (!storagePoolIdToIsoPrefix.containsKey(storagePoolId)) {
it's a philosophical discussion - whether the command should encapsulate all
the relevant things (including caching) or only do invoke the vdsm verb it
should invoke and be wrapped by another class.
Since it was that way before (in IrsBrokerCommand) and we all agree that this
command should be replaced (either the engine should compute the iso prefix by
himself or the engine will pass only the iso name and vdsm will create the full
path), then it is better to keep it that way for now and think about further
improvement later.
Line 28: synchronized(getLockObjForStoragePool(storagePoolId)) {
Line 29: if
(!storagePoolIdToIsoPrefix.containsKey(storagePoolId)) {
Line 30: try {
Line 31: StoragePoolInfoReturnForXmlRpc retVal =
Line 34: storagePoolIdToIsoPrefix.put(storagePoolId,
Line 35:
retVal.mStoragePoolInfo.containsKey(IrsProperties.isoPrefix) ?
Line 36:
retVal.mStoragePoolInfo.get(IrsProperties.isoPrefix).toString()
Line 37: : StringUtils.EMPTY);
Line 38: } catch (Exception ex) {
I'm not sure. you tell me - it's a storage related command that had this catch
statement before, it was copied from IrsBrokerCommand. I don't mind to remove
it but we need to be sure that it is not here because of some bug that was
detected in the past..
Line 39: log.errorFormat("IsoPrefix Failed to get
storage pool info (vds {0}).", getParameters().getVdsId());
Line 40: return StringUtils.EMPTY;
Line 41: }
Line 42: }
Line 44: }
Line 45: return storagePoolIdToIsoPrefix.get(storagePoolId);
Line 46: }
Line 47:
Line 48: static void clearCachedIsoPrefix(Guid storagePoolId) {
yeah, I recently removed the lock I took here at the begining. we'll have flows
where an ISO prefix will be returned when there is no active ISO domain anyway
- so I don't see why bother with complex synchronizations (which will result in
more context switching)
Line 49: storagePoolIdToIsoPrefix.remove(storagePoolId);
Line 50: }
Line 51:
Line 52: private static Object getLockObjForStoragePool(Guid storagePoolId)
{
--
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: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
