pbacsko commented on code in PR #840:
URL: https://github.com/apache/yunikorn-core/pull/840#discussion_r1561423983
##########
pkg/scheduler/partition.go:
##########
@@ -591,14 +590,12 @@ func (pc *PartitionContext) updatePartitionResource(delta
*resources.Resource) {
// This locks the partition. The partition may not be locked when we process
the allocation
// additions to the node as that takes further app, queue or node locks
func (pc *PartitionContext) addNodeToList(node *objects.Node) error {
- pc.Lock()
- defer pc.Unlock()
- // Node can be added to the system to allow processing of the
allocations
if err := pc.nodes.AddNode(node); err != nil {
return fmt.Errorf("failed to add node %s to partition %s,
error: %v", node.NodeID, pc.Name, err)
}
+ pc.Lock()
+ defer pc.Unlock()
Review Comment:
Actually we can move it back.
1) We already do something similar in `updatePartitionDetails()`. In
`updateNodeSortingPolicy()` we make reference to `pc.nodes` while having a
partition lock.
2) This can give an inconsistent view: node is already in the data
structure, however, partition information (`totalPartitionResource`,
`root.maxResource`) has been not updated yet. I don't think it's critical
though, but it can trigger the health checker in the wrong moment.
3) At the same time, in `PartitionContext.addAllocation()`, we update a
bunch of objects individually with fine grained locking. So at the end of day,
we can still end up having an inconsistent view anyway (for a brief moment) if
another goroutine comes along and acquires the partition lock.
Looks like it has become a matter of preference.
The most important thing is NOT to have partition lock when calling back
from the Application. We already achieved this task.
I'll let @wilfred-s share his opinion on this.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
