kingamarton commented on a change in pull request #301:
URL: 
https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r716659804



##########
File path: pkg/cache/placeholder_manager.go
##########
@@ -165,3 +167,16 @@ func (mgr *PlaceholderManager) isRunning() bool {
 func (mgr *PlaceholderManager) setRunning(flag bool) {
        mgr.running.Store(flag)
 }
+
+func (mgr *PlaceholderManager) getOrphanPodsLength() int {
+       mgr.Lock()
+       defer mgr.Unlock()

Review comment:
       Here a readlock is enough

##########
File path: pkg/cache/placeholder_manager.go
##########
@@ -165,3 +167,16 @@ func (mgr *PlaceholderManager) isRunning() bool {
 func (mgr *PlaceholderManager) setRunning(flag bool) {
        mgr.running.Store(flag)
 }
+
+func (mgr *PlaceholderManager) getOrphanPodsLength() int {
+       mgr.Lock()
+       defer mgr.Unlock()
+       orphanPodsLength := len(mgr.orphanPods)

Review comment:
       you don't need a variable for the length in this case. Just return it 
(`return len(mgr.orphanPods)`)

##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,41 @@ func TestPlaceholderManagerStartStop(t *testing.T) {
        time.Sleep(5 * time.Millisecond)
        assert.Equal(t, mgr.isRunning(), false, "placeholder manager has not 
stopped")
 }
+
+func TestPlaceholderManagerCleanup(t *testing.T) {
+       mockedAPIProvider := client.NewMockedAPIProvider()
+       pod1 := &v1.Pod{
+               TypeMeta: apis.TypeMeta{
+                       Kind:       "Pod",
+                       APIVersion: "v1",
+               },
+               ObjectMeta: apis.ObjectMeta{
+                       Name: "pod-01",
+                       UID:  "UID-01",
+               },
+       }
+       pod2 := &v1.Pod{
+               TypeMeta: apis.TypeMeta{
+                       Kind:       "Pod",
+                       APIVersion: "v1",
+               },
+               ObjectMeta: apis.ObjectMeta{
+                       Name: "pod-02",
+                       UID:  "UID-02",
+               },
+       }
+       mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+       mgr.setCleanupTime(100 * time.Millisecond)

Review comment:
       You can set it back to the original valuee right after you change it 
using `defer` statement, so you can be sure that wou will not forget to set it 
back.
   `mgr.setCleanupTime(100 * time.Millisecond)`
   `defer mgr.setCleanupTime(5 * time.Second)`
   
   




-- 
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]


Reply via email to