On Thu, Apr 6, 2017 at 11:26 AM Roy Golan <rgo...@redhat.com> wrote: > On Wed, Apr 5, 2017 at 11:34 PM Moti Asayag <masa...@redhat.com> wrote: > >> On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgo...@redhat.com> wrote: >> >> >> >> On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masa...@redhat.com> wrote: >> >> Hi All, >> >> ATM, there are 78 occurrences of "Injector.injectMembers(new >> AuditLogableBase())" in ovirt-engine project, which their main purpose is >> to ease the resolve of the placeholders of the audit log message while >> logging an event. >> >> For instance AuditLogType.MAC_ADDRESS_IS_EXTERNAL is being used from >> ImportVmCommandBase.java in the following way: >> >> private AuditLogableBase createExternalMacsAuditLog(VM vm, Set<String> >> externalMacs) { >> AuditLogableBase logable = *Injector.injectMembers*(new >> AuditLogableBase()); >> logable.setVmId(vm.getId()); >> logable.setCustomCommaSeparatedValues("MACAddr", externalMacs); >> return logable; >> } >> >> The entry in the properties file is: >> MAC_ADDRESS_IS_EXTERNAL=VM ${*VmName*} has MAC address(es) ${MACAddr}, >> which is/are out of its MAC pool definitions. >> >> Therefore the only purpose of the injection is to allow the >> AuditLogDirector to resolve the ${*VmName*} which is already known at >> the time of creating the AuditLogableBase entry. >> >> The result is injecting the DAOs for the AuditLogableBase instance and >> using the VM dao to retrieve the VM entry from the DB. >> This is just a wastef of injection and DB access while both can be spared. >> >> This could have been easily replaced by one of the following: >> >> - auditLogableBase.setVmName(vm.getName()); >> >> - setVmName is protected so not usable as is >> >> >> It will become public if we agree on >> >> >> https://gerrit.ovirt.org/#/c/75244/2/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java >> >> >> >> - auditLogableBase.addCustomValue("VmName", vm.getName()); >> >> I prefer this, it is readable. and BTW it is fluent, it returns 'this' so >> use >> >> AuditLogDirector(new AuditLogableBase(type) >> .addCustomValue("VmName", vm.getName())); >> >> >> I'm okay with this as well. >> >> >> Please pick up any occurrence from your domain and send a patch to >> replace it where possible. >> Thanks in advance, >> Moti >> >> >> +1 >> >> Frankly the fact that all the DAOs sets protected access in >> AuditLogableBase is a total abuse. Every command should declare its own >> deps. >> >> >> That will require a huge effort. >> > > Removed them all, https://gerrit.ovirt.org/75262 compile +1 > Now need to fix the tests - I'd appreciate help here > > Verified +1 the removal of all protected DAO's in AuditLogableBase. Instead of constant injection of *66* dao fields for *every* command inject only *9* which are most atm, They will be removed as well in the future.
Please help review, its a long, but stupid patch which just adds injections. https://gerrit.ovirt.org/75262 > >> >> _______________________________________________ >> >> Devel mailing list >> Devel@ovirt.org >> http://lists.ovirt.org/mailman/listinfo/devel >> >> >> >> >> -- >> Regards, >> Moti >> >
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel