wilfred-s commented on a change in pull request #248:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/248#discussion_r601184397
##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -220,27 +220,44 @@ func TestCleanOrphanPlaceholders(t *testing.T) {
UID: "UID-01",
},
}
- placeholderMgr.orphanPod["task01"] = pod1
- assert.Equal(t, len(placeholderMgr.orphanPod), 1)
+ placeholderMgr.orphanPods["task01"] = pod1
+ assert.Equal(t, len(placeholderMgr.orphanPods), 1)
placeholderMgr.cleanOrphanPlaceholders()
- assert.Equal(t, len(placeholderMgr.orphanPod), 0)
+ assert.Equal(t, len(placeholderMgr.orphanPods), 0)
}
func TestPlaceholderManagerStartStop(t *testing.T) {
mockedAPIProvider := client.NewMockedAPIProvider()
- placeholderMgr := &PlaceholderManager{
- clients: mockedAPIProvider.GetAPIs(),
- orphanPod: make(map[string]*v1.Pod),
- running: atomic.Value{},
- RWMutex: sync.RWMutex{},
- }
- placeholderMgr.setRunning(false)
+ mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+ assert.Equal(t, mgr.running.Load(), false, "new manager should not run")
// start clean up goroutine
- placeholderMgr.Start()
- assert.Equal(t, placeholderMgr.isRunning(), true)
+ mgr.Start()
+ assert.Equal(t, mgr.isRunning(), true, "manager should be running after
start")
+
+ // starting a second time should do nothing
+ mgr.Start()
+ assert.Equal(t, mgr.isRunning(), true, "sending start 2nd time should
not do anything")
+
+ // this is a blocking call
+ mgr.Stop()
+ // allow time for processing as the call can return faster than the
flag is set
+ time.Sleep(5 * time.Millisecond)
Review comment:
This sleep time has nothing to do with the sleep in the
PlaceholderManager. The sleep that is here is purely to allow the go routine to
process updating the stop flag so we can see that it has been done.
The `stopChan` is an unbuffered channel. The `mgr.Stop()` call is thus
blocking until the channel has been read. The channel read happens before the
flag gets set that is why we need to sleep in the test. The atomic update of
the flag must be done before we check. Without the small sleep in the test the
go routine might not have done that on a multi core machine when the test
progresses.
You can see that the call is blocking when you run the old code and the
test. It takes 8s to complete. When I replaced the `time.Sleep(3 *
time.Second)` in the old test with `time.Sleep(5 * time.Millisecond)` the test
finished in 5s. Similar when I changed the clean interval the test ran as long
as the clean interval was set to.
--
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]
