Moti Asayag has posted comments on this change.
Change subject: core: scheduling: increase NetworkPolicyUnit performance
......................................................................
Patch Set 4:
(11 comments)
The engine supports nics or bonds without networks attached to.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/NetworkPolicyUnit.java
Line 42: } else {
Line 43: return null;
Line 44: }
Line 45: Map<Guid, List<String>> hostNics =
Line 46:
getInterfaceDAO().getHostNetworksByCluster(hosts.get(0).getVdsGroupId());
hosts.get(0).getVdsGroupId() is being called couple of times, i think
extracting it into local parameter "clusterId" will ease understanding the code.
Line 47: for (VDS host : hosts) {
Line 48: VdcBllMessages returnValue =
Line 49: validateRequiredNetworksAvailable(host,
Line 50: vm,
Line 145: *
Line 146: * @param host
Line 147: * the host
Line 148: * @param onlyRequiredNetworks
Line 149: * should be false, in order the method to be
non-trivial.
:-)
Line 150: * @param hostNetworks
Line 151: * list of hosts networks names
Line 152: * @param allNetworksInCluster
Line 153: * @return <c>true</c> if the cluster's display network is
defined on the host or
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/InterfaceDaoDbFacadeImpl.java
Line 131: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource()
Line 132: .addValue("cluster_id", clusterId);
Line 133:
Line 134: List<Pair<Guid, String>> list =
Line 135:
getCallsHandler().executeReadList("GetHostNetworksByCluster", new
RowMapper<Pair<Guid, String>>() {
The convention is to have all mappers static in order to reduce their
instantiations as they have no state.
Please extract it into a private static class.
Line 136:
Line 137: @Override
Line 138: public Pair<Guid, String> mapRow(ResultSet rs,
int arg1) throws SQLException {
Line 139: return new Pair<Guid, String>(getGuid(rs,
"vds_id"), rs.getString("network_name"));
Line 139: return new Pair<Guid, String>(getGuid(rs,
"vds_id"), rs.getString("network_name"));
Line 140: }
Line 141: }, parameterSource);
Line 142:
Line 143: Map<Guid, List<String>> map = new HashMap<Guid,
List<String>>();
the type can be omitted from the instantiation: new HashMap<>();
please use meaningful name, such as neworksByHost
Line 144: if (list != null) {
Line 145: for (Pair<Guid, String> pair : list) {
Line 146: List<String> hostNameList = map.get(pair.getFirst());
Line 147: if (hostNameList == null) {
Line 140: }
Line 141: }, parameterSource);
Line 142:
Line 143: Map<Guid, List<String>> map = new HashMap<Guid,
List<String>>();
Line 144: if (list != null) {
the list will never be null, so this can be removed.
please use meaningful instead of list, such as hostNetworks
Line 145: for (Pair<Guid, String> pair : list) {
Line 146: List<String> hostNameList = map.get(pair.getFirst());
Line 147: if (hostNameList == null) {
Line 148: hostNameList = new ArrayList<>();
Line 142:
Line 143: Map<Guid, List<String>> map = new HashMap<Guid,
List<String>>();
Line 144: if (list != null) {
Line 145: for (Pair<Guid, String> pair : list) {
Line 146: List<String> hostNameList = map.get(pair.getFirst());
the hostNameList is actually hostNetworks. please rename.
Line 147: if (hostNameList == null) {
Line 148: hostNameList = new ArrayList<>();
Line 149: map.put(pair.getFirst(), hostNameList);
Line 150: }
Line 143: Map<Guid, List<String>> map = new HashMap<Guid,
List<String>>();
Line 144: if (list != null) {
Line 145: for (Pair<Guid, String> pair : list) {
Line 146: List<String> hostNameList = map.get(pair.getFirst());
Line 147: if (hostNameList == null) {
generally, this block can be modified a bit, no dramatic change:
if (!map.containsKey(pair.getFirst())) {
map.put(pair.getFirst(), new ArrayList<>());
}
map.get(pair.getFirst()).add(pair.getSecond());
}
it replaces 5 lines with 3 and i think a bit more readable.
Line 148: hostNameList = new ArrayList<>();
Line 149: map.put(pair.getFirst(), hostNameList);
Line 150: }
Line 151: hostNameList.add(pair.getSecond());
Line 146: List<String> hostNameList = map.get(pair.getFirst());
Line 147: if (hostNameList == null) {
Line 148: hostNameList = new ArrayList<>();
Line 149: map.put(pair.getFirst(), hostNameList);
Line 150: }
please add a space line after }
Line 151: hostNameList.add(pair.getSecond());
Line 152: }
Line 153: }
Line 154: return map;
Line 149: map.put(pair.getFirst(), hostNameList);
Line 150: }
Line 151: hostNameList.add(pair.getSecond());
Line 152: }
Line 153: }
please add a space line after }
Line 154: return map;
Line 155: }
Line 156:
Line 157: @Override
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/InterfaceDaoTest.java
Line 345: assertFalse(result.isEmpty());
Line 346: }
Line 347:
Line 348: @Test
Line 349: public void testGetAllInterfacesByVdsGroupId() {
the test name isn't accurate, should probably be testGetHostNetworksByCluster
Line 350: Map<Guid, List<String>> map =
dao.getHostNetworksByCluster(CLUSTER_ID);
Line 351: assertNotNull(map);
Line 352: assertEquals(1, map.size());
Line 353: assertEquals(1, map.get(VDS_ID).size());
Line 348: @Test
Line 349: public void testGetAllInterfacesByVdsGroupId() {
Line 350: Map<Guid, List<String>> map =
dao.getHostNetworksByCluster(CLUSTER_ID);
Line 351: assertNotNull(map);
Line 352: assertEquals(1, map.size());
i think that a strict assumption like this one will be less favourable for
maintenance: Any change which adds/removes a nic from the fixtures.xml will
trigger failure in this test.
Please use the following assertions instead of the existing one (compiled by
html ;-)) :
assertNotNull(map);
assertFalse(map.isEmpty());
assertNotNull(map.get(VDS_ID));
assertFalse(map.get(VDS_ID).isEmpty());
Line 353: assertEquals(1, map.get(VDS_ID).size());
Line 354: }
--
To view, visit http://gerrit.ovirt.org/22463
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b394b5d50abcbf58c63cb2d2af3867197ddb788
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[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
