Martin Mucha has posted comments on this change. Change subject: core: macPool per DC, db changes ......................................................................
Patch Set 16: (11 comments) answers http://gerrit.ovirt.org/#/c/26795/16/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacPool.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacPool.java: Line 8: Line 9: import org.hibernate.validator.constraints.NotEmpty; Line 10: import org.ovirt.engine.core.compat.Guid; Line 11: Line 12: public class MacPool implements BusinessEntity<Guid>, Commented, Serializable{ > Why do you need a comment? I don't need it. It was requested in our meeting. These notes were also send to everybody as a summary of that meeting. I'm just sticking to that. Line 13: private Guid id; Line 14: Line 15: @NotNull Line 16: private String name; Line 12: public class MacPool implements BusinessEntity<Guid>, Commented, Serializable{ Line 13: private Guid id; Line 14: Line 15: @NotNull Line 16: private String name; > Does a private pool has to have a name? not sure, probably not. If you're ok with that, I'll remove it. Line 17: Line 18: private boolean shared; Line 19: Line 20: private boolean allowDuplicateMacAddresses; Line 25: private Set<MacRange> ranges = new HashSet<MacRange>(); Line 26: Line 27: @Override Line 28: public Guid getId() { Line 29: return this.id; > No need to use "this" Done Line 30: } Line 31: Line 32: @Override Line 33: public void setId(Guid id) { http://gerrit.ovirt.org/#/c/26795/16/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacRange.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacRange.java: Line 5: import javax.validation.constraints.NotNull; Line 6: Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public class MacRange implements BusinessEntity<Guid>, Commented, Serializable{ > Why do you need it to be commented? I don't need it. It was requested in our meeting. These notes were also send to everybody as a summary of that meeting. I'm just sticking to that. Line 10: private Guid id; Line 11: private Guid macPoolId; Line 12: Line 13: @NotNull Line 10: private Guid id; Line 11: private Guid macPoolId; Line 12: Line 13: @NotNull Line 14: private String macFrom; //TODO MM: change to PgMacaddr > Why do you want to use PgMacaddr ? sorry I think this is class representing db object, like Hibernate entity. This TODO is obsolete. I've already agreed with Moti that on business layer he wants handling macs using String type. Line 15: Line 16: @NotNull Line 17: private String macTo; Line 18: private String comment; http://gerrit.ovirt.org/#/c/26795/16/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StoragePool.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StoragePool.java: Line 50: Line 51: private QuotaEnforcementTypeEnum quotaEnforcementType; Line 52: Line 53: @NotNull(groups = {CreateEntity.class, UpdateEntity.class}) Line 54: private MacPool macPool; > You should hold the ID of the pool only, no reason to fetch this data each a) isn't this a premature optimization? :) b) ok, I'd be just fine with this, since for cost of 2 inner joins (over very small tables) I get consistent data going to presentation layer. Defined this way, all can be solved in row mapper. Otherwise I'd have to analyze which calls need it and those call will have to fire extra query or row mapper would have to be defined in such way, that it can handle both. so what's mike's way? Do not optimize, extra query when needed, universal RowMapper or else? Line 55: Line 56: public StoragePool() { Line 57: id = Guid.Empty; Line 58: status = StoragePoolStatus.Uninitialized; http://gerrit.ovirt.org/#/c/26795/16/packaging/dbscripts/create_tables.sql File packaging/dbscripts/create_tables.sql: > Why change this file? Done http://gerrit.ovirt.org/#/c/26795/16/packaging/dbscripts/upgrade/03_05_0300_add_mac_pool_ranges_to_storage_pool.sql File packaging/dbscripts/upgrade/03_05_0300_add_mac_pool_ranges_to_storage_pool.sql: Line 5: allow_duplicate_mac_addresses BOOLEAN NOT NULL, Line 6: comment CHARACTER VARYING(255) Line 7: ); Line 8: Line 9: CREATE TABLE default_mac_pool ( > Why do you add this table? I think there is a request to select default pool and change it. Somebody requests it in our first meeting. I have it in mine notes from that meeting. But aside from that. Frankly I cannot see much consistency in "we don't want push bad code into our code base" (and I accept it, if you do not want 'this.' constructs in your code base, it's fine) and having in code constants for IDs of DB objects. When first ('this') is axiomatically code smell, lets say like a 12y old virgin fart whisper in a whirpool, then DB-ID constant in code is like 10 whales farting at same time in a concert hall in a canon(music term, ask lior if needed). This should not be very problematic to implement. Line 10: id UUID NOT NULL PRIMARY KEY, Line 11: mac_pool_id UUID NOT NULL REFERENCES mac_pool (id) ON DELETE RESTRICT Line 12: ); Line 13: Line 13: Line 14: CREATE TABLE mac_range ( Line 15: id UUID NOT NULL PRIMARY KEY, Line 16: mac_pool_id UUID NOT NULL REFERENCES mac_pool (id) ON DELETE CASCADE, Line 17: from_mac MACADDR NOT NULL, > Is this type supported by our DAL? postgres jdbc driver handles conversions to string back and forth. Moti requested this datatype; locks us to postres, but I do believe that we're already locked in. I guess it's fine then. Line 18: to_mac MACADDR NOT NULL, Line 19: comment CHARACTER VARYING(255) Line 20: ); Line 21: Line 24: INSERT INTO mac_pool (id, name, shared, allow_duplicate_mac_addresses, comment) SELECT Line 25: uuid_generate_v1(), Line 26: 'Default', Line 27: TRUE, Line 28: FALSE, > You should take the actual value from the config option. can you help me with that? I do not know how to access engine config from db update script. Just point me to some code fragment/wiki/whatever, please. Line 29: 'Default shared MAC pool'; Line 30: Line 31: INSERT INTO default_mac_pool (id, mac_pool_id) select uuid_generate_v1(), ((select id from mac_pool limit 1)); Line 32: Line 33: INSERT INTO mac_range (id, mac_pool_id, from_mac, to_mac) SELECT Line 34: uuid_generate_v1(), Line 35: (select mac_pool_id from default_mac_pool limit 1), Line 36: '00:1a:4a:4:f1:00', Line 37: '00:1a:4a:4:f1:ff'; > You should take the actual ranges from the config option and insert them al same. Line 38: Line 39: update storage_pool set mac_pool_id=(select mac_pool_id from default_mac_pool limit 1); Line 40: alter table storage_pool alter COLUMN mac_pool_id SET NOT NULL; -- To view, visit http://gerrit.ovirt.org/26795 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id30f3c384ecf933daaacdbdd6542e88afb98f7ca Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [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
