Moti Asayag has posted comments on this change.
Change subject: core: When updating entity name the event...
......................................................................
Patch Set 1: (11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 543: }
Line 544:
Line 545: private void logRenamedEntity() {
Line 546: if (this instanceof RenamedEntityInfoProvider) {
Line 547: String entity="UNKNOWN";
there is no need to initialize the entity, oldName and newName as being
overridden straightafterward in the try-catch block.
I would even move the their declaration into th try block. e.g.:
try {
String entity = ReflectionUtils.getMethodValue(this,
"getEntityName").toString();
...
}
Or as Yair suggested:
try {
if (this instanceof RenamedEntityProvider) {
RenamedEntityProvider renameable = (RenamedEntityProvider)this;
String entity = renameable.getEntityName();
...
renameable.setId(logable);
}
...
}
Line 548: String oldName="";
Line 549: String newName="";
Line 550: try {
Line 551: entity = ReflectionUtils.getMethodValue(this,
"getEntityName").toString();
Line 553: newName = ReflectionUtils.getMethodValue(this,
"getNewName").toString();
Line 554: if (!oldName.equals(newName)) {
Line 555: // log entity rename details
Line 556: AuditLogableBase logable = new AuditLogableBase();
Line 557: logable.AddCustomValue("EntityName", entity);
s/EntityName/EntityType ?
Line 558: logable.AddCustomValue("OldName", oldName);
Line 559: logable.AddCustomValue("NewName", newName);
Line 560: Class<AuditLogableBase> partypes[] = new Class[1];
Line 561: partypes[0] = AuditLogableBase.class;
Line 555: // log entity rename details
Line 556: AuditLogableBase logable = new AuditLogableBase();
Line 557: logable.AddCustomValue("EntityName", entity);
Line 558: logable.AddCustomValue("OldName", oldName);
Line 559: logable.AddCustomValue("NewName", newName);
The next 6 lines could be replaced by:
renameable.setId(logable);
Line 560: Class<AuditLogableBase> partypes[] = new Class[1];
Line 561: partypes[0] = AuditLogableBase.class;
Line 562: Object arglist[] = new Object[1];
Line 563: arglist[0] = logable;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RenamedEntityInfoProvider.java
Line 1: package org.ovirt.engine.core.bll;
Line 2:
Line 3: import
org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogableBase;
Line 4:
Line 5: public interface RenamedEntityInfoProvider {
How about naming this interface as Renameable, Make it extends the Nameable
interface that provides the current name of an entity ?
Line 6:
Line 7: public String getEntityName();
Line 8: public String getOldName();
Line 9: public String getNewName();
Line 3: import
org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogableBase;
Line 4:
Line 5: public interface RenamedEntityInfoProvider {
Line 6:
Line 7: public String getEntityName();
maybe this should method be called: getEntityType since it reveals the type of
the entity. the *Name method's suffix should be minimized for clarity IMO.
Line 8: public String getOldName();
Line 9: public String getNewName();
Line 10: public void setId(AuditLogableBase logable);
Line 6:
Line 7: public String getEntityName();
Line 8: public String getOldName();
Line 9: public String getNewName();
Line 10: public void setId(AuditLogableBase logable);
probably should be renamed to setEntityId as it sets the id of the renamed
entity
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsCommand.java
Line 209: return super.getValidationGroups();
Line 210: }
Line 211:
Line 212: @Override
Line 213: public String getEntityName() {
You can user VdcObjectType.VDS.getVdcObjectTranslation() which already have the
proper representation of the entity type (same goes for other entities) instead
of retyping them so we maintain uniformity as well.
Line 214: return "Host";
Line 215: }
Line 216:
Line 217: @Override
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
Line 54: initDwhSeverities();
Line 55: initConfigSeverities();
Line 56: initUserAccountSeverities();
Line 57: checkSeverities();
Line 58: initGeneral();
the call for checkSeverities(); should be the last call from within this method.
Since this smells, could you please move it to the static block, after the call
to initSeverities() ? It would be a proper solution for this.
Line 59: }
Line 60:
Line 61: private static void initGlusterVolumeSeverities() {
Line 62: mSeverities.put(AuditLogType.GLUSTER_VOLUME_CREATE,
AuditLogSeverity.NORMAL);
Line 706: mSeverities.put(AuditLogType.USER_ACCOUNT_DISABLED_OR_LOCKED,
AuditLogSeverity.ERROR);
Line 707: mSeverities.put(AuditLogType.USER_ACCOUNT_PASSWORD_EXPIRED,
AuditLogSeverity.ERROR);
Line 708: }
Line 709:
Line 710: private static void initGeneral() {
please maintain method naming convention: initGeneralServirities
In addition, i think that 'general' is .... too general :-)
But I can't come up with a better name for now, maybe initCommonSevirities ?
Line 711: mSeverities.put(AuditLogType.ENTITY_RENAMED,
AuditLogSeverity.NORMAL);
Line 712: }
Line 713: private static void initDwhSeverities() {
Line 714: mSeverities.put(AuditLogType.DWH_STOPPED,
AuditLogSeverity.NORMAL);
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 536: RELOAD_CONFIGURATIONS_SUCCESS=System Configurations reloaded
successfully.
Line 537: RELOAD_CONFIGURATIONS_FAILURE=System Configurations failed to reload.
Line 538: USER_ACCOUNT_DISABLED_OR_LOCKED=User ${UserName} cannot login, as it
got disabled or locked. Please contact the system administrator.
Line 539: USER_ACCOUNT_PASSWORD_EXPIRED=User ${UserName} cannot login, as the
user account password has expired. Please contact the system administrator.
Line 540: ENTITY_RENAMED=${EntityName} ${OldName} was renamed from ${OldName}
to ${NewName}.
s/EntityName/EntityType ?
Line 541: STORAGE_DOMAIN_TASKS_ERROR=Storage Domain ${StorageDomainName} is
down while there are tasks running on it. These tasks may fail.
Line 542: IMPORTEXPORT_IMPORT_VM_INVALID_INTERFACES=While importing VM
${VmName}, the Network/s ${Networks} were found to be Non-VM Networks or do not
exist in Cluster. Network Name was not set in the Interface/s ${Interfaces}.
Line 543: IMPORTEXPORT_IMPORT_TEMPLATE_INVALID_INTERFACES=While importing
Template ${VmTemplateName}, the Network/s ${Networks} were found to be Non-VM
Networks or do not exist in Cluster. Network Name was not set in the
Interface/s ${Interfaces}.
Line 544: VDS_SET_NON_OPERATIONAL_VM_NETWORK_IS_BRIDGELESS=Host ${VdsName} does
not comply with the cluster ${VdsGroupName} networks, the following VM networks
are bridgeless: '${Networks}'
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ReflectionUtils.java
Line 106: return annotation;
Line 107: }
Line 108: }
Line 109: return null;
Line 110: }
Seems the changes within this file can be reverted
Line 111:
Line 112: /**
Line 113: * Gets a method (without parametrs) return value
Line 114: *
--
To view, visit http://gerrit.ovirt.org/11756
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17f0049ca0ffb1a3868ba404b112a8c10ee7778e
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
