Martin Mucha has uploaded a new change for review.
Change subject: core: refactor; encapsulation
......................................................................
core: refactor; encapsulation
• Map<Guid, Whatever> is only mapping Guid to Whatever. It does not
guarantee, that Guid is ID of Whatever. This should be somehow
strongly enforced; it's not safe passing mappings around forcing
callee to trust caller, that he did mapping right. Touched files was
not interrested in created Maps itself ~ true usecase was: get network
for iface and get network for attachment.
• I don't know in what number grade the network count will be, but
passing Map/NetworkBy around seems like premature optimization to me.
Change-Id: Ic42fb58302a66995f366227e43950d2722c77b33
Signed-off-by: Martin Mucha <mmu...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java
A
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/NetworkBy.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidator.java
6 files changed, 58 insertions(+), 32 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/43/36143/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java
index 3251f27..8fc6b50 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java
@@ -77,7 +77,7 @@
expectedAttachments.add(getParameters().getNetworkAttachment());
NetworkAttachmentsValidator crossAttachmentsValidator =
new NetworkAttachmentsValidator(expectedAttachments,
-
Entities.businessEntitiesById(getNetworkDAO().getAllForCluster(getVdsGroupId())));
+ new
NetworkBy(getNetworkDAO().getAllForCluster(getVdsGroupId())));
return
validate(crossAttachmentsValidator.validateNetworkExclusiveOnNics());
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java
index 75ad969..e1bc777 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java
@@ -42,7 +42,6 @@
import org.ovirt.engine.core.common.vdscommands.VDSCommandType;
import org.ovirt.engine.core.common.vdscommands.VDSReturnValue;
import
org.ovirt.engine.core.common.vdscommands.VdsIdAndVdsVDSCommandParametersBase;
-import org.ovirt.engine.core.compat.Guid;
import org.ovirt.engine.core.compat.Version;
import org.ovirt.engine.core.dao.network.HostNetworkQosDao;
import org.ovirt.engine.core.utils.NetworkUtils;
@@ -59,7 +58,8 @@
Arrays.asList(VDSStatus.Maintenance, VDSStatus.Up,
VDSStatus.NonOperational);
private static final Logger log =
LoggerFactory.getLogger(HostSetupNetworksCommand.class);
private HostSetupNetworksValidator validator;
- private Map<Guid, Network> clusterNetworks;
+ private NetworkBy networkBy;
+
private Set<String> removedNetworks;
private Set<String> removedBonds;
private List<VdsNetworkInterface> existingNics;
@@ -107,7 +107,7 @@
getParameters(),
getExistingNics(),
getExistingAttachments(),
- getClusterNetworks());
+ networkBy());
List<String> validationMessages = validator.validate();
if (!validationMessages.isEmpty()) {
@@ -225,10 +225,9 @@
if (existingNics == null) {
existingNics =
getDbFacade().getInterfaceDao().getAllInterfacesForVds(getVdsId());
HostNetworkQosDao qosDao = getDbFacade().getHostNetworkQosDao();
- Map<String, Network> networksByName =
Entities.entitiesByName(getClusterNetworks().values());
for (VdsNetworkInterface iface : existingNics) {
- Network network = networksByName.get(iface.getNetworkName());
+ Network network = networkBy().iface(iface);
iface.setNetworkImplementationDetails(NetworkUtils.calculateNetworkImplementationDetails(network,
network == null ? null :
qosDao.get(network.getQosId()),
iface));
@@ -250,19 +249,11 @@
if (removedNetworks == null) {
removedNetworks = new
HashSet<>(getParameters().getRemovedNetworkAttachments().size());
for (NetworkAttachment attachment :
getParameters().getRemovedNetworkAttachments()) {
-
removedNetworks.add(getClusterNetworks().get(attachment.getNetworkId()).getName());
+
removedNetworks.add(networkBy().attachment(attachment).getName());
}
}
return removedNetworks;
- }
-
- private Map<Guid, Network> getClusterNetworks() {
- if (clusterNetworks == null) {
- clusterNetworks =
Entities.businessEntitiesById(getNetworkDAO().getAllForCluster(getVdsGroupId()));
- }
-
- return clusterNetworks;
}
private List<HostNetwork> getNetworks() {
@@ -271,7 +262,7 @@
BusinessEntityMap<VdsNetworkInterface> nics = new
BusinessEntityMap<>(getExistingNics());
for (NetworkAttachment attachment :
getParameters().getNetworkAttachments()) {
- Network network =
getClusterNetworks().get(attachment.getNetworkId());
+ Network network = networkBy().attachment(attachment);
HostNetwork networkToConfigure = new HostNetwork(network,
attachment);
networkToConfigure.setBonding(isBonding(attachment, nics));
@@ -329,7 +320,7 @@
private List<Network> getModifiedNetworks() {
List<Network> modifiedNetworks = new
ArrayList<>(getParameters().getNetworkAttachments().size());
for (NetworkAttachment attachment :
getParameters().getNetworkAttachments()) {
-
modifiedNetworks.add(getClusterNetworks().get(attachment.getNetworkId()));
+ modifiedNetworks.add(networkBy().attachment(attachment));
}
return modifiedNetworks;
@@ -358,4 +349,13 @@
}
});
}
+
+ private NetworkBy networkBy() {
+ if (networkBy == null) {
+ networkBy = new
NetworkBy(getNetworkDAO().getAllForCluster(getVdsGroupId()));
+ }
+
+ return networkBy;
+ }
+
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java
index f341fcc..9f216ee 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java
@@ -47,7 +47,7 @@
private Map<String, VdsNetworkInterface> existingIfaces;
private List<NetworkAttachment> existingAttachments;
private boolean networkCustomPropertiesSupported;
- private Map<Guid, Network> clusterNetworks;
+ private NetworkBy networkBy;
private final Map<Guid, NetworkAttachment> attachmentsById;
private final BusinessEntityMap<Bond> removedBonds;
@@ -55,12 +55,12 @@
HostSetupNetworksParameters params,
List<VdsNetworkInterface> existingNics,
List<NetworkAttachment> existingAttachments,
- Map<Guid, Network> clusterNetworks) {
+ NetworkBy networkBy) {
this.host = host;
this.params = params;
this.existingAttachments = existingAttachments;
this.existingIfaces = Entities.entitiesByName(existingNics);
- this.clusterNetworks = clusterNetworks;
+ this.networkBy = networkBy;
setSupportedFeatures();
@@ -103,7 +103,7 @@
private ValidationResult
validateNetworkExclusiveOnNics(Collection<NetworkAttachment>
attachmentsToConfigure) {
NetworkAttachmentsValidator validator =
- new NetworkAttachmentsValidator(attachmentsToConfigure,
clusterNetworks);
+ new NetworkAttachmentsValidator(attachmentsToConfigure,
networkBy);
return validator.validateNetworkExclusiveOnNics();
}
@@ -112,7 +112,7 @@
Set<Guid> networkIds = new HashSet<>(attachmentsToConfigure.size());
for (NetworkAttachment attachment : attachmentsToConfigure) {
if (networkIds.contains(attachment.getNetworkId())) {
- Network network =
clusterNetworks.get(attachment.getNetworkId());
+ Network network = networkBy.attachment(attachment);
addViolation(VdcBllMessages.NETWORKS_ALREADY_ATTACHED_TO_IFACES,
network.getName());
passed = false;
} else {
@@ -127,7 +127,7 @@
boolean passed = true;
Collection<String> removedNetworks = new HashSet<String>();
for (NetworkAttachment removedAttachment :
params.getRemovedNetworkAttachments()) {
-
removedNetworks.add(clusterNetworks.get(removedAttachment.getNetworkId()).getName());
+
removedNetworks.add(networkBy.attachment(removedAttachment).getName());
}
List<String> vmNames =
getVmInterfaceManager().findActiveVmsUsingNetworks(host.getId(),
removedNetworks);
@@ -328,7 +328,7 @@
private boolean notRemovingLabeledNetworks(NetworkAttachment attachment,
Map<String, VdsNetworkInterface> existingNics) {
- Network removedNetwork =
clusterNetworks.get(attachment.getNetworkId());
+ Network removedNetwork = networkBy.attachment(attachment);
if (!NetworkUtils.isLabeled(removedNetwork)) {
return true;
}
@@ -392,7 +392,7 @@
nicsToNetworks.put(nicName, new ArrayList<Network>());
}
- Network networkToConfigure =
clusterNetworks.get(attachment.getNetworkId());
+ Network networkToConfigure = networkBy.attachment(attachment);
nicsToNetworks.get(nicName).add(networkToConfigure);
}
return nicsToNetworks;
@@ -420,7 +420,7 @@
Map<String, String> validPropertiesNonVm = new HashMap<String,
String>(validProperties);
validPropertiesNonVm.remove("bridge_opts");
for (NetworkAttachment attachment : params.getNetworkAttachments()) {
- Network network = clusterNetworks.get(attachment.getNetworkId());
+ Network network = networkBy.attachment(attachment);
if (attachment.hasProperties()) {
if (!networkCustomPropertiesSupported) {
addViolation(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_CUSTOM_PROPERTIES_NOT_SUPPORTED,
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/NetworkBy.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/NetworkBy.java
new file mode 100644
index 0000000..064a2d3
--- /dev/null
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/NetworkBy.java
@@ -0,0 +1,26 @@
+package org.ovirt.engine.core.bll.network.host;
+
+import java.util.List;
+
+import org.ovirt.engine.core.common.businessentities.BusinessEntityMap;
+import org.ovirt.engine.core.common.businessentities.network.Network;
+import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment;
+import
org.ovirt.engine.core.common.businessentities.network.VdsNetworkInterface;
+
+//TODO MM: or rather inline all this class? It'll be less verbose but still ok.
+public class NetworkBy {
+ private final BusinessEntityMap<Network> map;
+
+
+ public NetworkBy(List<Network> networks) {
+ map = new BusinessEntityMap<>(networks);
+ }
+
+ public Network attachment(NetworkAttachment attachment) {
+ return map.get(attachment.getNetworkId());
+ }
+
+ public Network iface(VdsNetworkInterface iface) {
+ return map.get(iface.getNetworkName());
+ }
+}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java
index 7ab646d..6ad23fa 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkAttachmentCommand.java
@@ -68,7 +68,7 @@
List<NetworkAttachment> expectedAttachments =
getExpectedNetworkAttachments();
NetworkAttachmentsValidator crossAttachmentsValidator =
new NetworkAttachmentsValidator(expectedAttachments,
-
Entities.businessEntitiesById(getNetworkDAO().getAllForCluster(getVdsGroupId())));
+ new
NetworkBy(getNetworkDAO().getAllForCluster(getVdsGroupId())));
return
validate(crossAttachmentsValidator.validateNetworkExclusiveOnNics());
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidator.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidator.java
index 5cc2c57..5ea937a 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidator.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidator.java
@@ -9,10 +9,10 @@
import org.apache.commons.collections.bag.HashBag;
import org.ovirt.engine.core.bll.ValidationResult;
+import org.ovirt.engine.core.bll.network.host.NetworkBy;
import org.ovirt.engine.core.common.businessentities.network.Network;
import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment;
import org.ovirt.engine.core.common.errors.VdcBllMessages;
-import org.ovirt.engine.core.compat.Guid;
import org.ovirt.engine.core.utils.NetworkUtils;
/**
@@ -23,12 +23,12 @@
public class NetworkAttachmentsValidator {
private final Collection<NetworkAttachment> attachmentsToConfigure;
- private final Map<Guid, Network> clusterNetworks;
+ private final NetworkBy networkBy;
public NetworkAttachmentsValidator(Collection<NetworkAttachment>
attachmentsToConfigure,
- Map<Guid, Network> clusterNetworks) {
+ NetworkBy networkBy) {
this.attachmentsToConfigure = attachmentsToConfigure;
- this.clusterNetworks = clusterNetworks;
+ this.networkBy = networkBy;
}
public ValidationResult validateNetworkExclusiveOnNics() {
@@ -39,7 +39,7 @@
nicsToNetworks.put(nicName, new ArrayList<NetworkType>());
}
- Network networkToConfigure =
clusterNetworks.get(attachment.getNetworkId());
+ Network networkToConfigure = networkBy.attachment(attachment);
nicsToNetworks.get(nicName).add(determineNetworkType(networkToConfigure));
}
--
To view, visit http://gerrit.ovirt.org/36143
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic42fb58302a66995f366227e43950d2722c77b33
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <mmu...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
