Roy Golan has posted comments on this change.

Change subject: core: VM Monitoring abstract fetching/analyzing/monitoring
......................................................................


Patch Set 19:

(3 comments)

http://gerrit.ovirt.org/#/c/28662/19/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java:

Line 287:             ).perform();
Line 288:         }
Line 289:     }
Line 290: 
Line 291:     public VDS getVds() {
> can you please explain why this is needed? i thought you are going to delet
just keeping the cacheVds immutable by external usages.

the intention is to hand a copy of vds to the ListFetcher because it is 
required by VdsBroker commands. I'm not sure
those commands won't mess with it.

will change the method name and add javadoc
Line 292:         //TODO use deep clone for defensive copying. Make VDS 
implement Clonable
Line 293:         return cachedVds.clone();
Line 294:     }
Line 295:     public String getVdsName() {


Line 305:     }
Line 306: 
Line 307:     private void logFailureMessage(RuntimeException ex) {
Line 308:         log.warn(
Line 309:                 "Failed to refresh VDS , cachedVds = '{}' : '{}', 
error = '{}', continuing.",
> please change message to use "vds" and not "cachedVds"
Done
Line 310:                 cachedVds.getName(),
Line 311:                 cachedVds.getId());
Line 312:         log.error("Exception", ex);
Line 313:     }


Line 307:     private void logFailureMessage(RuntimeException ex) {
Line 308:         log.warn(
Line 309:                 "Failed to refresh VDS , cachedVds = '{}' : '{}', 
error = '{}', continuing.",
Line 310:                 cachedVds.getName(),
Line 311:                 cachedVds.getId());
> you are missing the ex.getMsg
Done
Line 312:         log.error("Exception", ex);
Line 313:     }
Line 314: 
Line 315:     private static void logException(final RuntimeException ex) {


-- 
To view, visit http://gerrit.ovirt.org/28662
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1adf0a95007140e89b080b5160ba93e340ee3ba6
Gerrit-PatchSet: 19
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Roy Golan <rgo...@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

Reply via email to