Martin Mucha has posted comments on this change. Change subject: core: added missing logging + refactoring ......................................................................
Patch Set 1: (4 comments) answers. —— comment: when presence of keys is enforced by unit test, I could simplify few methods. question 1: shouldn't we turn this class full of static methods into proper class? question 2: I do believe, that we should run test scanning for all keys presence for all resource bundle locales. Currently there's just one, but I'd expect, that there can be more translations. question 3: ok, so property file was invalid with following keys missing. Do you know who to ask for proper translations? UNASSIGNED VDS_AUTO_FENCE_STATUS VDS_AUTO_FENCE_STATUS_FAILED VDS_AUTO_FENCE_FAILED_CALL_FENCE_SPM VDS_HIGH_NETWORK_USE USER_FAILED_REMOVE_VM USER_RUN_UNLOCK_ENTITY_SCRIPT VDS_NETWORK_MTU_DIFFER_FROM_LOGICAL_NETWORK VDS_HOST_IN_CONNECTING_STATE STORAGE_ACTIVATE_ASYNC DWH_STOPPED DWH_STARTED DWH_ERROR USER_REMOVE_AUDIT_LOG USER_REMOVE_AUDIT_LOG_FAILED USER_CLEAR_ALL_DISMISSED_AUDIT_LOG USER_CLEAR_ALL_DISMISSED_AUDIT_LOG_FAILED http://gerrit.ovirt.org/#/c/29244/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java: Line 112: AuditLogSeverity severity = logType.getSeverity(); Line 113: Line 114: //TODO MM: shouldn't this be rather warning? This assumption is hardly valid. Line 115: //TODO MM: there's NO such situation when this can happen; and since we find this invalid anyway, it should be 'blocked' in AuditLogType constructor. Line 116: if (severity == null) { > this condition is no longer valid. there will always be a severity to audit I can be wrong, but how's that possible? Severity is set via one of constructors in AuditLogType, where null can be passed. AuditLogType has not checkings that severity is not null, therefore it can be null, and that null can be passed into this method. Am I right? Line 117: severity = AuditLogSeverity.NORMAL; Line 118: log.infoFormat("No severity for {0} audit log type, assuming Normal severity", logType); Line 119: } Line 120: AuditLog auditLog = createAuditLog(auditLogable, logType, loggerString, severity); Line 121: Line 122: if (auditLog == null) { Line 123: log.warn("Unable to create AuditLog"); Line 124: } else { Line 125: auditLog.setstorage_domain_id(auditLogable.getStorageDomainId()); > this could also be extracted into a method. Done Line 126: auditLog.setstorage_domain_name(auditLogable.getStorageDomainName()); Line 127: auditLog.setstorage_pool_id(auditLogable.getStoragePoolId()); Line 128: auditLog.setstorage_pool_name(auditLogable.getStoragePoolName()); Line 129: auditLog.setvds_group_id(auditLogable.getVdsGroupId()); Line 143: logMessage(severity, getMessageToLog(loggerString, auditLog)); Line 144: } Line 145: } Line 146: Line 147: private static void logMessage(AuditLogSeverity severity, String logMessage) { > since it resides on the common project, i don't think it is doable (as it r ok, I've overlooked that this is bound to gwt. That it has to stay in this form. Line 148: //TODO MM: rather than this, there could(should?) be OO approach: introducing 'log' method on AuditLogSeverity. Line 149: switch (severity) { Line 150: case NORMAL: Line 151: log.info(logMessage); Line 191: auditLogable.getEventFloodInSec(), Line 192: auditLogable.getCustomData()); Line 193: } Line 194: Line 195: final String messageByType = messages.get(logType); > i'd enforce this as part of the build (via unit test) so any entry in Aduit Done Line 196: if (messageByType == null) { Line 197: return null; Line 198: } else { Line 199: // Application log message from AuditLogMessages -- To view, visit http://gerrit.ovirt.org/29244 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic737ace1808e1f242d0eb08ee458869a89be500e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@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
