Martin Mucha has posted comments on this change. Change subject: core: audit logging support for mac pools. ......................................................................
Patch Set 7: (3 comments) http://gerrit.ovirt.org/#/c/29204/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java: Line 64: setSucceeded(true); Line 65: getReturnValue().setActionReturnValue(getMacPoolId()); Line 66: } Line 67: Line 68: //used by introspector > Is there an option to add those methods to MacPoolCommandBase and override not really I think, since RemoveMacPoolCommand does not use same parameter as others, namely it's parameter does not have name. I've tried to extract it there, but did not find common behavior of those three commands I could build getId and getName on. Code duplicity is better than flawed hierarchy, so that the reason why I did it this way. But if you see a way how to do that, I'll change it. Line 69: public Guid getMacPoolId() { Line 70: return getMacPoolEntity().getId(); Line 71: } Line 72: http://gerrit.ovirt.org/#/c/29204/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLog.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLog.java: Line 9: import org.ovirt.engine.core.compat.Guid; Line 10: Line 11: public class AuditLog extends IVdcQueryable implements Serializable { Line 12: private static final long serialVersionUID = -5346124122179332184L; Line 13: > please remove this change from the patch. why? if we're explicitely specifying generated serialVersionUID it should be valid or not present. This number has meaning, that all serialized instances of this number can be deserialized into class using of this number. Which is certainly not true in this case. This newer class instance cannot be deserialized into older class. That's why I changed number. Common approaches are: a) specify number manually and update it when needed (this is the case, I do believe); specified number has calculated by proper tool for this. b) do not specify that field at all, and compiller will do it for you automatically. c) some (so called) tutors/experts recommends using 1L constant, defying deserializing procedure, which has consequence, that deserializing process will fail if class is not compatible, but same would happen (but sooner in process) if they don't mention the field at all. (argument supporting this is that even minor change in class, like property order, changes the number and in that case is deserialization improperly evaluated as unsuccessful; but playing with numbers isn't worth fixing this problem, since this kind of deserialization should not be used for long term data storage). any a-c option is fine I think(although 'c' is quite stupid), but specifying manual number and not updating it on change is ... I cannot see any benefit in doing this. If we want "wrong and not updated serialVersionUID" we should stick to 1L, since this way is it at least obvious, that this number is wrong. Line 14: private long auditLogId; Line 15: private Date logTime; Line 16: private String message; Line 17: private Guid userId; http://gerrit.ovirt.org/#/c/29204/7/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 835: KDUMP_FLOW_DETECTED_ON_VDS=Kdump flow detected on host '${VdsName}'. Line 836: KDUMP_FLOW_NOT_DETECTED_ON_VDS=Kdump flow not detected on host '${VdsName}'. Line 837: KDUMP_FLOW_FINISHED_ON_VDS=Kdump flow finished on host '${VdsName}'. Line 838: KDUMP_DETECTION_NOT_CONFIGURED_ON_VDS=Kdump detection is enabled for host '${VdsName}', but kdump is not configured properly on host. Line 839: > s/created/was created ? if agreed, same change should go to other messages Done Line 840: MAC_POOL_ADD_SUCCESS=MAC Pool '${MacPoolName}' (id=${MacPoolId}) created. Line 841: MAC_POOL_ADD_FAILED=Failed to create MAC Pool '${MacPoolName}'. Line 842: MAC_POOL_EDIT_SUCCESS=MAC Pool '${MacPoolName}' (id=${MacPoolId}) changed. Line 843: MAC_POOL_EDIT_FAILED=Failed to update MAC Pool '${MacPoolName}' (id=${MacPoolId}). -- To view, visit http://gerrit.ovirt.org/29204 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I376b89abc03657a7cd2eb1b06e21591e4cd944ad Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
