Martin Mucha has posted comments on this change.

Change subject: core: DI for AuditLoggerDirector
......................................................................


Patch Set 7:

(2 comments)

https://gerrit.ovirt.org/#/c/33901/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceVdsBaseCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceVdsBaseCommand.java:

Line 115:                 log.info("Power-Management: {} host '{}' failed.", 
getAction(), getVdsName());
Line 116:                 audit(AuditLogType.FENCE_OPERATION_FAILED);
Line 117:             }
Line 118:         } catch(Exception e) {
Line 119:             e.printStackTrace(System.err);
> why is it here ?
shit. I was debugging one test which threw exception probably in some mock and 
it was swallowed here making outcome of test really weird. I found this 
swallowing, so I needed to understand what exception it is and why it's thrown. 
I however definitely did not want to push this, thanks for noticing me.

But when we're at it, I may be worth it, to throw exception here or at least 
add logging of that exception. I'm not able to decide if any exception is not 
important at all here. I'd think contrary, but ... Please let me know, if you 
want that fixed.

printing to std remove. Done.
Line 120:         } finally {
Line 121:             if (!getSucceeded()) {
Line 122:                 setStatus(lastStatus);
Line 123:                 if (!wasSkippedDueToPolicy(result)) {


https://gerrit.ovirt.org/#/c/33901/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmsFromExternalProviderQuery.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmsFromExternalProviderQuery.java:

Line 17: 
Line 18: public class GetVmsFromExternalProviderQuery<T extends 
GetVmsFromExternalProviderQueryParameters> extends QueriesCommandBase<T> {
Line 19: 
Line 20:     @Inject
Line 21:     private AuditLogDirector auditLogDirector;
> is it required if already added to QueriesCommandBase ?
Overlooked. Done.
Line 22: 
Line 23:     public GetVmsFromExternalProviderQuery(T parameters) {
Line 24:         this(parameters, null);
Line 25:     }


-- 
To view, visit https://gerrit.ovirt.org/33901
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifab56339a634234bbc51fa09719fb87b5cc69501
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <[email protected]>
Gerrit-Reviewer: Martin Mucha <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: mooli tayer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to