You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/09/13 06:48:51 UTC

[GitHub] [incubator-yunikorn-k8shim] kobe860219 opened a new pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

kobe860219 opened a new pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301


   ### What is this PR for?
   Before this PR, the [code](https://github.com/apache/incubator-yunikorn-k8shim/blob/9df2f67e3d955a5d2de36cc0540cc86268a9d683/pkg/cache/placeholder_manager.go#L146) has not be tested yet. Add an unit test for up it test coverage.
   
   
   ### What type of PR is it?
   * [ ] - Bug Fix
   * [x] - Improvement
   * [ ] - Feature
   * [ ] - Documentation
   * [ ] - Hot Fix
   * [ ] - Refactoring
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   https://issues.apache.org/jira/browse/YUNIKORN-552
   
   ### How should this be tested?
   1. go test ./pkg/cache -cover -race -tags deadlock -coverprofile=coverage.txt -covermode=atomic
   2. go tool cover -html=coverage.txt
   
   ### Screenshots (if appropriate)
   ![image](https://user-images.githubusercontent.com/48027290/133036608-74ab852f-5796-4a32-a236-7f1eae8d5821.png)
   
   
   ### Questions:
   * [ ] - The licenses files need update.
   * [ ] - There is breaking changes for older versions.
   * [ ] - It needs documentation.
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-927116434


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#301](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (230dadc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `3.57%`.
   > The diff coverage is `75.99%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #301      +/-   ##
   ==========================================
   + Coverage   59.75%   63.32%   +3.57%     
   ==========================================
     Files          35       37       +2     
     Lines        3133     3493     +360     
   ==========================================
   + Hits         1872     2212     +340     
   - Misses       1180     1192      +12     
   - Partials       81       89       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/cache/context\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `45.23% <0.00%> (-1.11%)` | :arrow_down: |
   | [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `36.52% <0.00%> (+2.36%)` | :arrow_up: |
   | [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGVzLmdv) | `78.18% <ø> (-1.63%)` | :arrow_down: |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/states.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvc3RhdGVzLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/shim/main.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NoaW0vbWFpbi5nbw==) | `0.00% <ø> (ø)` | |
   | ... and [32 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9df2f67...230dadc](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r709941454



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()

Review comment:
       Added




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r709940928



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1

Review comment:
       ![image](https://user-images.githubusercontent.com/48027290/133586240-76eb3b45-834c-48a6-b8af-d45790bf129a.png)
   




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] 0yukali0 edited a comment on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
0yukali0 edited a comment on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-922491753


   Hi @kobe860219 
   1.orphanPods is accessed by  cleanOrphanPlaceholders and len(mgr.orphanPods) same time.
   You need a lock to protect this variable.
   > func (mgr *PlaceholderManager) GetorphanPodsLength {
     mgr.Lock()
     defer mgr.Unlock()
     return len(mgr.orphanPods)
   }
   
   2.cleanupTime is also accessed  to write and read same time.
   make sure this variable is accessed by only one process at one time.
   > func (mgr *PlaceholderManager) GetCleanupTime {
     mgr.Lock()
     defer mgr.Unlock()
     return mgr.cleanupTime
   }
   
   > func (mgr *PlaceholderManager) SetCleanupTime {
     mgr.Lock()
     defer mgr.Unlock()
     mgr.cleanupTime = 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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r709941166



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(mgr.orphanPods), 1)
+	<-time.After(5 * time.Second)

Review comment:
       ![image](https://user-images.githubusercontent.com/48027290/133586284-26abf199-96c1-44dd-896d-90a20e4dcbef.png)
   




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] 0yukali0 commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
0yukali0 commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-922491753


   Hi @kobe860219 
   1.orphanPods is accessed by  cleanOrphanPlaceholders and len(mgr.orphanPods) same time.
   You need a lock to protect this variable.
   > func (mgr *PlaceholderManager) GetorphanPodsLength {
     mgr.Lock()
     defer mgr.Unlock()
     return len(mgr.orphanPods)
   }
   
   2.cleanupTime is also accessed  to write and read same time.
   make sure this variable is accessed by only one process at one time.
   > func (mgr *PlaceholderManager) GetCleanupTime {
     mgr.Lock()
     defer mgr.Unlock()
     return mgr.cleanupTime
   }
   
   func (mgr *PlaceholderManager) SetCleanupTime {
     mgr.Lock()
     defer mgr.Unlock()
     mgr.cleanupTime = 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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-927116434


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#301](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2005ffe) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `3.55%`.
   > The diff coverage is `76.03%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #301      +/-   ##
   ==========================================
   + Coverage   59.75%   63.30%   +3.55%     
   ==========================================
     Files          35       37       +2     
     Lines        3133     3483     +350     
   ==========================================
   + Hits         1872     2205     +333     
   - Misses       1180     1190      +10     
   - Partials       81       88       +7     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/cache/context\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `45.23% <0.00%> (-1.11%)` | :arrow_down: |
   | [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `36.52% <0.00%> (+2.36%)` | :arrow_up: |
   | [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGVzLmdv) | `79.80% <ø> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/states.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvc3RhdGVzLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/shim/main.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NoaW0vbWFpbi5nbw==) | `0.00% <ø> (ø)` | |
   | ... and [30 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9df2f67...2005ffe](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r717775163



##########
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:
       Thanks ! This tip is very helpful for me !

##########
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:
       I will remove unessential variable.

##########
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:
       Thanks for your review. I replaced `Mute` with `RWMute` for `RLock()` in `PlaceholderManager` struct.




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r717776488



##########
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:
       I will remove unessential variable.




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-927116530


   @yangwwei  Hi, Could you help me review it. Thanks!


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r717775163



##########
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:
       Thanks ! This tip is very helpful for me !




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-920733636


   ![image](https://user-images.githubusercontent.com/48027290/133586204-257046f7-5b76-4e0e-8aca-a6049d6dfc01.png)
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 removed a comment on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 removed a comment on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-920733636


   ![image](https://user-images.githubusercontent.com/48027290/133586204-257046f7-5b76-4e0e-8aca-a6049d6dfc01.png)
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] wilfred-s closed pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301


   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r709941251



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1

Review comment:
       Added




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-917891349


   @yangwwei  @HuangTing-Yao @0yukali0 Please help me review this pr. Thanks !


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r707519227



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()

Review comment:
       can we make sure mgr is stopped after the test?

##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(mgr.orphanPods), 1)
+	<-time.After(5 * time.Second)

Review comment:
       we can't wait for 5s in UT for this single test
   what we can do here is to add a function that can change the default 5s interval for the placeholder cleanup, e.g `setCleanupInterval(interval time.Duration)`. During this test, set the interval to 100millsecond, etc. And remember to set it back to 5s after the test.

##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ 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",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1

Review comment:
       can we add more than 1 pod here?




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-936509340


   > @yangwwei Hi, thanks your help for rerun the check. The e2e test now is ok !
   
   That seems like an intermittent issue, it looks good now.
   
   @kingamarton could u pls give another look and see if this is good to go?


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-927116434


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#301](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (230dadc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `3.57%`.
   > The diff coverage is `75.99%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #301      +/-   ##
   ==========================================
   + Coverage   59.75%   63.32%   +3.57%     
   ==========================================
     Files          35       37       +2     
     Lines        3133     3493     +360     
   ==========================================
   + Hits         1872     2212     +340     
   - Misses       1180     1192      +12     
   - Partials       81       89       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/cache/context\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `45.23% <0.00%> (-1.11%)` | :arrow_down: |
   | [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `36.52% <0.00%> (+2.36%)` | :arrow_up: |
   | [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGVzLmdv) | `78.18% <ø> (-1.63%)` | :arrow_down: |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/states.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvc3RhdGVzLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/shim/main.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NoaW0vbWFpbi5nbw==) | `0.00% <ø> (ø)` | |
   | ... and [33 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9df2f67...230dadc](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-927116434


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#301](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (230dadc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `3.57%`.
   > The diff coverage is `75.99%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #301      +/-   ##
   ==========================================
   + Coverage   59.75%   63.32%   +3.57%     
   ==========================================
     Files          35       37       +2     
     Lines        3133     3493     +360     
   ==========================================
   + Hits         1872     2212     +340     
   - Misses       1180     1192      +12     
   - Partials       81       89       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/cache/context\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `45.23% <0.00%> (-1.11%)` | :arrow_down: |
   | [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `36.52% <0.00%> (+2.36%)` | :arrow_up: |
   | [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGVzLmdv) | `78.18% <ø> (-1.63%)` | :arrow_down: |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/states.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9ldmVudHMvc3RhdGVzLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/shim/main.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NoaW0vbWFpbi5nbw==) | `0.00% <ø> (ø)` | |
   | ... and [33 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9df2f67...230dadc](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r717777898



##########
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:
       Thanks for your review. I replaced `Mute` with `RWMute` for `RLock()` in `PlaceholderManager` struct.




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] kobe860219 commented on pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

Posted by GitBox <gi...@apache.org>.
kobe860219 commented on pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#issuecomment-936121618


   @yangwwei  Hi, thanks your help for rerun the check. The e2e test now is ok !


-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org