wilfred-s commented on code in PR #1004:
URL: https://github.com/apache/yunikorn-core/pull/1004#discussion_r1891075035
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -3438,3 +3438,130 @@ func TestApplication_canAllocationReserve(t *testing.T)
{
})
}
}
+
+func TestTryPlaceHolderAllocateNoPlaceHolders(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ask := newAllocationAsk(aKey, appID0, res)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since there are
no placeholders to allocate")
+}
+
+func TestTryPlaceHolderAllocateSmallerRequest(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is smaller than placeholder
+ smallerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 1})
+ ask := newAllocationAsk(aKey, appID0, smallerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result != nil, "result should not be nil since the ask
is smaller than the placeholder")
Review Comment:
Extend the checks of the result to be more than just not nil: what is the
type returned etc.
What happens with the placeholder data?
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -3438,3 +3438,130 @@ func TestApplication_canAllocationReserve(t *testing.T)
{
})
}
}
+
+func TestTryPlaceHolderAllocateNoPlaceHolders(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ask := newAllocationAsk(aKey, appID0, res)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since there are
no placeholders to allocate")
+}
+
+func TestTryPlaceHolderAllocateSmallerRequest(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is smaller than placeholder
+ smallerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 1})
+ ask := newAllocationAsk(aKey, appID0, smallerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result != nil, "result should not be nil since the ask
is smaller than the placeholder")
+}
+
+func TestTryPlaceHolderAllocateLargerRequest(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is larger than placeholder
+ largerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 10})
+ ask := newAllocationAsk(aKey, appID0, largerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since the ask is
larger than the placeholder")
Review Comment:
Extend the checks of the result to be more than just nil. What happens with
the placeholder data? What happens to the placeholder?
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -3438,3 +3438,130 @@ func TestApplication_canAllocationReserve(t *testing.T)
{
})
}
}
+
+func TestTryPlaceHolderAllocateNoPlaceHolders(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ask := newAllocationAsk(aKey, appID0, res)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since there are
no placeholders to allocate")
+}
+
+func TestTryPlaceHolderAllocateSmallerRequest(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is smaller than placeholder
+ smallerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 1})
+ ask := newAllocationAsk(aKey, appID0, smallerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result != nil, "result should not be nil since the ask
is smaller than the placeholder")
+}
+
+func TestTryPlaceHolderAllocateLargerRequest(t *testing.T) {
+ const tg1 = "tg-1"
+
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is larger than placeholder
+ largerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 10})
+ ask := newAllocationAsk(aKey, appID0, largerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since the ask is
larger than the placeholder")
+}
+
+func TestTryPlaceHolderAllocateDifferentTaskGroups(t *testing.T) {
+ const (
+ tg1 = "tg-1"
+ tg2 = "tg-2"
+ )
Review Comment:
Would be good to refactor this now as we have this same const declaration in
at least 3 places in the file. Create one const for the file.
Can be done as part of this PR
--
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]
