Roy Golan has posted comments on this change.

Change subject: engine: scheduling optimization field
......................................................................


Patch Set 6:

(3 comments)

Generally looks ok.
the flow is now running concurrently instead of sync. I've try to drill down 
the code
to trace shared state. 

just one thing - the filters are sorted in flow. is this relevant that they 
should be filltered
over and over again? also since the filter are shared, future changes to the 
sort will not play nice here. the data structure is not thread safe.

just clear out this. I'll continue on with the review

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/SchedulingManager.java
Line 262:             if (policy.getFunctions() == null || 
policy.getFunctions().isEmpty()) {
Line 263:                 return vdsList.get(0).getId();
Line 264:             }
Line 265:             Guid bestHost = null;
Line 266:             // weigh hosts iff there are more than 1 host in case 
optimize for speed
typo iff/if
Line 267:             // is true and there are more than configurable requests 
pending skip weighing
Line 268:             if (vdsList.size() > 1
Line 269:                     && (!cluster.isSchedulerOptimizeForSpeed()
Line 270:                     || 
clusterLockMap.get(cluster.getId()).getQueueLength() <=


Line 267:             // is true and there are more than configurable requests 
pending skip weighing
Line 268:             if (vdsList.size() > 1
Line 269:                     && (!cluster.isSchedulerOptimizeForSpeed()
Line 270:                     || 
clusterLockMap.get(cluster.getId()).getQueueLength() <=
Line 271:                     Config.<Integer> 
GetValue(ConfigValues.OptimizeSchedulerForSpeedPendingRequests))) {
conig value name is not explantory: 
please concider
ParallelSchedulingRequestThreshold  instead

also wrapping this withing a method would be clearer

 if (shoudWeighClusterHosts(vdsList, cluster)) {
     runFunctions...   
 }

 /*
 * the comment from the code here
 */
 shoudWeighClusterHosts(...){ ...}

also add logs if we skipped and add debug log on why
Line 272:                 bestHost = runFunctions(policy.getFunctions(), 
vdsList, vm, parameters);
Line 273:             }
Line 274:             if (bestHost == null && vdsList.size() > 0) {
Line 275:                 bestHost = vdsList.get(0).getId();


Line 284:                         vm.getNumOfCpus());
Line 285:             }
Line 286:             return bestHost;
Line 287:         } catch (InterruptedException e) {
Line 288:             // ignore
log it
Line 289:             return null;
Line 290:         } finally {
Line 291:             clusterLockMap.get(cluster.getId()).release();
Line 292:         }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0784f89648093e650be73fcc3d850a4854d53c1a
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
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