wilfred-s commented on a change in pull request #229:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/229#discussion_r540588990
##########
File path: pkg/scheduler/partition.go
##########
@@ -586,6 +586,16 @@ func (pc *PartitionContext) removeNode(nodeID string)
[]*objects.Allocation {
return pc.removeNodeInternal(nodeID)
}
+// Update a node
Review comment:
Explain what you update on the node and why that triggers the partition
update etc.
##########
File path: pkg/scheduler/partition.go
##########
@@ -586,6 +586,16 @@ func (pc *PartitionContext) removeNode(nodeID string)
[]*objects.Allocation {
return pc.removeNodeInternal(nodeID)
}
+// Update a node
+func (pc *PartitionContext) updateNode(node *objects.Node, newCapacity
*resources.Resource) {
+ pc.Lock()
+ defer pc.Unlock()
+ pc.totalPartitionResource.AddTo(newCapacity)
+ pc.totalPartitionResource.SubFrom(node.GetCapacity())
+ pc.root.SetMaxResource(pc.totalPartitionResource)
+ node.SetCapacity(newCapacity)
Review comment:
This is pretty lock heavy and might take locks that are not needed at
all.
Can we instead change the return of `node.SetCapacity()` to return a
`*resources.Resource` and return the change delta? If that delta not zero or
not nil we do not update the partition and root queue. Calling
`node.SetCapacity()` outside the partition lock and keep the time we hold the
partition lock to a minimum.
##########
File path: pkg/scheduler/tests/operation_test.go
##########
@@ -378,6 +383,67 @@ partitions:
assert.Equal(t, int64(node1.GetCapacity().Resources[resources.VCORE]),
int64(10))
assert.Equal(t,
int64(schedulingNode1.GetAllocatedResource().Resources[resources.MEMORY]),
int64(0))
assert.Equal(t,
int64(schedulingNode1.GetAvailableResource().Resources[resources.MEMORY]),
int64(300))
+
+ newRes, err = resources.NewResourceFromConf(map[string]string{"memory":
"300", "vcore": "10"})
+ assert.NilError(t, err, "failed to create resource")
+ assert.Equal(t, newRes.DAOString(),
partitionInfo.GetTotalPartitionResource().DAOString())
+ assert.Equal(t, newRes.DAOString(),
partitionInfo.GetQueue("root").GetMaxResource().DAOString())
+
+ // Register a node
+ err = ms.proxy.Update(&si.UpdateRequest{
Review comment:
We have a node already, we can perform the second part of the test
(lowering the resources) using the same node.
##########
File path: pkg/scheduler/tests/operation_test.go
##########
@@ -352,6 +352,11 @@ partitions:
assert.Equal(t,
int64(schedulingNode1.GetAllocatedResource().Resources[resources.MEMORY]),
int64(0))
assert.Equal(t,
int64(schedulingNode1.GetAvailableResource().Resources[resources.MEMORY]),
int64(100))
+ newRes, err :=
resources.NewResourceFromConf(map[string]string{"memory": "100", "vcore": "20"})
+ assert.NilError(t, err, "failed to create resource")
+ assert.Equal(t, newRes.DAOString(),
partitionInfo.GetTotalPartitionResource().DAOString())
+ assert.Equal(t, newRes.DAOString(),
partitionInfo.GetQueue("root").GetMaxResource().DAOString())
Review comment:
Instead of using string comparisons use direct resource compare call
either per value you want to check or by using resources.Equals() as is done
throughout the test. The string comparison does not add anything extra.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
