Moti Asayag has posted comments on this change. Change subject: core: Introduce NetworkAttachment entity ......................................................................
Patch Set 19: (3 comments) https://gerrit.ovirt.org/#/c/32411/19/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java: Line 22: private Guid networkId; Line 23: Line 24: private Guid nicId; Line 25: Line 26: // TODO: Add validation for the name for nic or bond > note: all mine TODOs has format: "//TODO MM: .*" Others are not mine todos. i'll try to reply on any TODOs which were added by me. I think this TODO isn't required. The nic name should be validated when adding the bond, which isn't part of this class. The only validation performed later is that nic should be exist on the host. so this TODO should be removed. Line 27: private String nicName; Line 28: Line 29: private IpConfiguration ipConfiguration; Line 30: private Map<String, String> properties; Line 107: return false; Line 108: if (getClass() != obj.getClass()) Line 109: return false; Line 110: NetworkAttachment other = (NetworkAttachment) obj; Line 111: if (id == null) { > 'equals' and hashcode are automatically generated methods by IDE. Please te with the transition to ORM all we should care of is the real identifiers of this class, which is id. however, before we'll move to ORM, i think that it would be wiser to have [id, network-id, nic id|name] as part of the equals. But that leads to other issues: what if one class has nic-id and the other only the nic-name. Those 2 class should be equal but this check would fail. So let's revert the thought, and let's settle for [id, network-id] which shouldn't be mutable once the class was created. an example could be taken from org.ovirt.engine.core.common.businessentities.EngineBackupLog class. please notice both usages of Objects.hash(...) and Objects.equals() for hashCode() and equals() methods. Line 112: if (other.id != null) Line 113: return false; Line 114: } else if (!id.equals(other.id)) Line 115: return false; Line 141: return true; Line 142: } Line 143: Line 144: @Override Line 145: public String toString() { recently mperina had introduced a new class named ToStringBuilder, resides in the common project. Please have a look at VnicProfile.toString() and implement it the same way for system consistency format of that method's output. Same should be applied for any new business/parameters classes which overrides toString() method. Line 146: return "NetworkAttachment [id=" + id Line 147: + ", networkId=" + networkId Line 148: + ", nicId=" + nicId Line 149: + ", nicName=" + nicName -- To view, visit https://gerrit.ovirt.org/32411 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ided81dc2be68dc4c7a9d491697f887cdae477a2c Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
