Maor Lipchuk has posted comments on this change.
Change subject: engine, dao: Introduce ISCSI Bond entity, tables and dao
......................................................................
Patch Set 3:
(10 comments)
missing Dao tests
What about permissions?
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 314:
Line 315: // ISCSI Bonds
Line 316: AddIscsiBond(2000, ActionGroup.CONFIGURE_VM_STORAGE, false,
QuotaDependency.NONE),
Line 317: EditIscsiBond(2001, ActionGroup.CONFIGURE_VM_STORAGE, false,
QuotaDependency.NONE),
Line 318: RemoveIscsiBond(2002, ActionGroup.CONFIGURE_VM_STORAGE, false,
QuotaDependency.NONE);
You are adding new action types to non existing commands.
It hsould be in the patch where you add the commands.
Line 319:
Line 320: private int intValue;
Line 321: private ActionGroup actionGroup;
Line 322: private boolean isActionMonitored;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/IscsiBond.java
Line 84: @Override
Line 85: public Object getQueryableId() {
Line 86: return getId();
Line 87: }
Line 88: }
What about Equals and hashCode. You will need them for the DAO tests
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/IscsiBondDao.java
Line 4:
Line 5: import org.ovirt.engine.core.common.businessentities.IscsiBond;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: public interface IscsiBondDao extends DAO {
You already have save,remove,update in ModificationDao. Get and GetAll are in
ReadDAO, why not simply extend GenericDAO which extends them both.
Line 9:
Line 10: public IscsiBond get(Guid id);
Line 11:
Line 12: public void save(IscsiBond iscsiBond);
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/IscsiBondDaoDbFacadeImpl.java
Line 51: }
Line 52:
Line 53: @Override
Line 54: public List<Guid> getNetworkIdsByIscsiBondId(Guid iscsiBondId) {
Line 55: return
getCallsHandler().executeReadList("GetNetworksByIscsiBondId", new
RowMapper<Guid>() {
I don't understand, why you didn't used IscsiBondRowMapper,instance
Line 56: @Override
Line 57: public Guid mapRow(ResultSet rs, int index) throws
SQLException {
Line 58: return new Guid((UUID) rs.getObject(1));
Line 59: }
Line 60: }, getCustomMapSqlParameterSource().addValue("iscsi_bond_id",
iscsiBondId));
Line 61: }
Line 62:
Line 63: @Override
Line 64: public void addNetwork(Guid iscsiBondId, Guid networkId) {
suggestion: I think that addNetworkToBond sounds more accurate and clear
Line 65:
getCallsHandler().executeModification("InsertNetworkToIscsiBond",
Line 66: getCustomMapSqlParameterSource()
Line 67: .addValue("iscsi_bond_id", iscsiBondId)
Line 68: .addValue("network_id", networkId));
Line 68: .addValue("network_id", networkId));
Line 69: }
Line 70:
Line 71: @Override
Line 72: public void removeNetwork(Guid iscsiBondId, Guid networkId) {
suggestion: I think that removeNetworkToBond sounds more accurate and clear
Line 73:
getCallsHandler().executeModification("RemoveNetworkFromIscsiBond",
Line 74: getCustomMapSqlParameterSource()
Line 75: .addValue("iscsi_bond_id", iscsiBondId)
Line 76: .addValue("network_id", networkId));
....................................................
File packaging/dbscripts/iscsi_bonds_sp.sql
Line 24:
Line 25: Create or replace FUNCTION GetNetworksByIscsiBondId(v_iscsi_bond_id
UUID) RETURNS SETOF UUID STABLE
Line 26: AS $procedure$
Line 27: BEGIN
Line 28: RETURN QUERY SELECT iscsi_bonds_networks_map.network_id
suggestion: iscsi_bonds_networks_map. can be removed from select, there is only
one table
Line 29: FROM iscsi_bonds_networks_map
Line 30: WHERE iscsi_bond_id = v_iscsi_bond_id;
Line 31: END; $procedure$
Line 32: LANGUAGE plpgsql;
Line 67: -- Get (and keep) a shared lock with "right to upgrade to
exclusive"
Line 68: -- in order to force locking parent before children
Line 69: SELECT id INTO v_val FROM iscsi_bonds WHERE id = v_id FOR UPDATE;
Line 70: DELETE FROM iscsi_bonds_networks_map WHERE iscsi_bond_id = v_id;
Line 71: DELETE FROM iscsi_bonds WHERE id = v_id;
You have a delete cascade on iscsi_bonds to iscsi_bonds_networks_map. you dont
need to delete iscsi_bonds_networks_map also
Line 72: END; $procedure$
Line 73: LANGUAGE plpgsql;
Line 74:
Line 75:
....................................................
File packaging/dbscripts/upgrade/03_04_0340_add_iscsi_bond.sql
Line 2: (
Line 3: id UUID NOT NULL,
Line 4: name varchar(50) NOT NULL,
Line 5: description varchar(4000),
Line 6: storage_pool_id UUID NOT NULL,
actually storage pool id is a bit redundant since the network table has this
field also, but I think that for the sake of decoupling maybe it's worth to
keep it.
Line 7: CONSTRAINT PK_iscsi_bonds PRIMARY KEY(id)
Line 8: ) WITH OIDS;
Line 9:
Line 10: ALTER TABLE iscsi_bonds ADD CONSTRAINT FK_iscsi_bonds_storage_pool
FOREIGN KEY(storage_pool_id)
Line 20: ALTER TABLE iscsi_bonds_networks_map ADD CONSTRAINT
FK_iscsi_bonds_networks_map_iscsi_bond_id FOREIGN KEY(iscsi_bond_id)
Line 21: REFERENCES iscsi_bonds(id) ON DELETE CASCADE;
Line 22:
Line 23: ALTER TABLE iscsi_bonds_networks_map ADD CONSTRAINT
FK_iscsi_bonds_networks_map_network_id FOREIGN KEY(network_id)
Line 24: REFERENCES network(id) ON DELETE CASCADE;
What will happen if we will remove a storage pool.
should we cascade delete the bond to? should we need to keep the bond orphan?
--
To view, visit http://gerrit.ovirt.org/22951
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I12313c02810a2f0e75016bdd78b44da43f2154d4
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[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
