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 2020/12/16 15:59:37 UTC

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao opened a new pull request #213: [YUNIKORN-478]Handle app completion at the shim side

HuangTing-Yao opened a new pull request #213:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/213


   Delete pod when receive `ReleaseAllocation` response, and delete application from context when receive `UpdatedApplication` response.
   Please review @yangwwei 
   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.

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

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on a change in pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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



##########
File path: pkg/cache/application_events.go
##########
@@ -160,3 +162,34 @@ func (fe FailApplicationEvent) GetArgs() []interface{} {
 func (fe FailApplicationEvent) GetApplicationID() string {
 	return fe.applicationID
 }
+
+// ------------------------
+// Release application
+// ------------------------
+type ReleaseApplicationEvent struct {
+	applicationID  string
+	allocationUUID string
+	event          events.ApplicationEventType
+}

Review comment:
       OK, done




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (bfb4fc0) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.05%`.
   > The diff coverage is `85.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.43%   +0.05%     
   ==========================================
     Files          32       32              
     Lines        2825     2859      +34     
   ==========================================
   + Hits         1621     1642      +21     
   - Misses       1137     1147      +10     
   - Partials       67       70       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `60.46% <90.90%> (+10.46%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.95% <100.00%> (+0.81%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `66.97% <100.00%> (-4.52%)` | :arrow_down: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...bfb4fc0](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (3d35b84) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.16%`.
   > The diff coverage is `69.23%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.54%   +0.16%     
   ==========================================
     Files          32       32              
     Lines        2825     2857      +32     
   ==========================================
   + Hits         1621     1644      +23     
   - Misses       1137     1144       +7     
   - Partials       67       69       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.59% <0.00%> (-0.55%)` | :arrow_down: |
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `58.53% <88.88%> (+8.53%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `71.62% <100.00%> (+0.13%)` | :arrow_up: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...7cca13a](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on a change in pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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



##########
File path: pkg/cache/context.go
##########
@@ -552,6 +552,16 @@ func (ctx *Context) RemoveApplication(appID string) error {
 	}
 }
 
+func (ctx *Context) RemoveApplicationInternal(appID string) error {

Review comment:
       OK, done

##########
File path: pkg/cache/application_events.go
##########
@@ -160,3 +162,34 @@ func (fe FailApplicationEvent) GetArgs() []interface{} {
 func (fe FailApplicationEvent) GetApplicationID() string {
 	return fe.applicationID
 }
+
+// ------------------------
+// Release application

Review comment:
       done.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on a change in pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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



##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -118,15 +118,36 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 	for _, release := range response.ReleasedAllocations {
 		log.Logger().Debug("callback: response to released allocations",
 			zap.String("UUID", release.UUID))
+
+		// delete the allocated pod
+		if app := callback.context.GetApplicationInternal(release.ApplicationID); app != nil {
+			// TerminationType 0 mean STOPPED_BY_RM
+			if release.TerminationType != si.AllocationRelease_STOPPED_BY_RM {
+				for _, task := range app.GetTaskMap() {
+					if task.GetTaskAllocationUUID() == release.UUID {
+						err := task.DeleteTaskPod(task.GetTaskPod())
+						if err != nil {
+							log.Logger().Error("failed to delete pod", zap.Error(err))
+						}
+					}
+				}
+			}
+		}
 	}
 
 	// handle status changes
 	for _, updated := range response.UpdatedApplications {
 		log.Logger().Debug("status update callback received",
 			zap.String("appId", updated.ApplicationID),
 			zap.String("new status", updated.State))
-
-		//handle status update
+		// delete application from context
+		if updated.State == events.States().Application.Completed {
+			err := callback.context.RemoveApplicationInternal(updated.ApplicationID)
+			if err != nil {
+				log.Logger().Error("failed to delete application", zap.Error(err))
+			}
+		}
+		// handle status update
 		dispatcher.Dispatch(cache.NewApplicationStatusChangeEvent(updated.ApplicationID, events.AppStateChange, updated.State))

Review comment:
       OK, done.

##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -118,15 +118,36 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 	for _, release := range response.ReleasedAllocations {
 		log.Logger().Debug("callback: response to released allocations",
 			zap.String("UUID", release.UUID))
+
+		// delete the allocated pod
+		if app := callback.context.GetApplicationInternal(release.ApplicationID); app != nil {
+			// TerminationType 0 mean STOPPED_BY_RM
+			if release.TerminationType != si.AllocationRelease_STOPPED_BY_RM {
+				for _, task := range app.GetTaskMap() {
+					if task.GetTaskAllocationUUID() == release.UUID {
+						err := task.DeleteTaskPod(task.GetTaskPod())
+						if err != nil {
+							log.Logger().Error("failed to delete pod", zap.Error(err))
+						}
+					}
+				}
+			}
+		}

Review comment:
       OK, done.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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



##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -118,15 +118,36 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 	for _, release := range response.ReleasedAllocations {
 		log.Logger().Debug("callback: response to released allocations",
 			zap.String("UUID", release.UUID))
+
+		// delete the allocated pod
+		if app := callback.context.GetApplicationInternal(release.ApplicationID); app != nil {
+			// TerminationType 0 mean STOPPED_BY_RM
+			if release.TerminationType != si.AllocationRelease_STOPPED_BY_RM {
+				for _, task := range app.GetTaskMap() {
+					if task.GetTaskAllocationUUID() == release.UUID {
+						err := task.DeleteTaskPod(task.GetTaskPod())
+						if err != nil {
+							log.Logger().Error("failed to delete pod", zap.Error(err))
+						}
+					}
+				}
+			}
+		}

Review comment:
       let's use event handling to handle this, so we do not need to expose `GetApplicationInternal ()`,  `GetTaskMap()` functions. We can change this to:
   
   ```
   ev := cache.NewReleaseAppAllocationEvent(app.GetApplicationID(), toReleaseAllocations, events.ReleaseAppAllocation)
   dispatcher.Dispatch(ev)
   ```
   
   in `application.go`, we just need to add another event `ReleaseAppAllocation`, and that triggers state transition from `Running` to `Running`. And in the handler, we can do the task pod cleanup.  

##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -118,15 +118,36 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 	for _, release := range response.ReleasedAllocations {
 		log.Logger().Debug("callback: response to released allocations",
 			zap.String("UUID", release.UUID))
+
+		// delete the allocated pod
+		if app := callback.context.GetApplicationInternal(release.ApplicationID); app != nil {
+			// TerminationType 0 mean STOPPED_BY_RM
+			if release.TerminationType != si.AllocationRelease_STOPPED_BY_RM {
+				for _, task := range app.GetTaskMap() {
+					if task.GetTaskAllocationUUID() == release.UUID {
+						err := task.DeleteTaskPod(task.GetTaskPod())
+						if err != nil {
+							log.Logger().Error("failed to delete pod", zap.Error(err))
+						}
+					}
+				}
+			}
+		}
 	}
 
 	// handle status changes
 	for _, updated := range response.UpdatedApplications {
 		log.Logger().Debug("status update callback received",
 			zap.String("appId", updated.ApplicationID),
 			zap.String("new status", updated.State))
-
-		//handle status update
+		// delete application from context
+		if updated.State == events.States().Application.Completed {
+			err := callback.context.RemoveApplicationInternal(updated.ApplicationID)
+			if err != nil {
+				log.Logger().Error("failed to delete application", zap.Error(err))
+			}
+		}
+		// handle status update
 		dispatcher.Dispatch(cache.NewApplicationStatusChangeEvent(updated.ApplicationID, events.AppStateChange, updated.State))

Review comment:
       after we delete the app in line 145, it continues to handle the event in line 151. this seems to be problematic. 
   should we add an `else` 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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   hi @HuangTing-Yao  any updates? If you have more questions, pls let me know. 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.

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 #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (7cca13a) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.40%`.
   > The diff coverage is `85.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.78%   +0.40%     
   ==========================================
     Files          32       32              
     Lines        2825     2859      +34     
   ==========================================
   + Hits         1621     1652      +31     
   - Misses       1137     1138       +1     
   - Partials       67       69       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `60.46% <90.90%> (+10.46%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.95% <100.00%> (+0.81%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `71.62% <100.00%> (+0.13%)` | :arrow_up: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...c4cafea](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (3d35b84) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.16%`.
   > The diff coverage is `69.23%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.54%   +0.16%     
   ==========================================
     Files          32       32              
     Lines        2825     2857      +32     
   ==========================================
   + Hits         1621     1644      +23     
   - Misses       1137     1144       +7     
   - Partials       67       69       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.59% <0.00%> (-0.55%)` | :arrow_down: |
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `58.53% <88.88%> (+8.53%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `71.62% <100.00%> (+0.13%)` | :arrow_up: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...3d35b84](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   hi @HuangTing-Yao  there is some issue here. First, we need to update go.mod file in order to use the latest binary from scheduler-interface repo, where we get the new `UpdateResponse#AllocationRelease`. And one more thing I forgot to mention, from the doc:
   ```
    // Released allocations, this could be either ack from scheduler when RM asks to terminate some allocations.
     // Or it could be decision made by scheduler (such as preemption or timeout).
     repeated AllocationRelease releasedAllocations = 3;
   ```
   we need to handle this differently based on the different TERMINATION_TYPE, 
   
   ```
    enum TerminationType {
       STOPPED_BY_RM = 0;          // Stopped or killed by ResourceManager (created by RM)
       TIMEOUT = 1;                // Timed out based on the executionTimeoutMilliSeconds (created by core)
       PREEMPTED_BY_SCHEDULER = 2; // Preempted allocation by scheduler (created by core)
       PLACEHOLDER_REPLACED = 3;   // Placeholder allocation replaced by real allocation (created by core)
     }
   ```
   
   if `STOPPED_BY_RM`,  there is no operation needed in the shim side (today's behavior)
   if otherwise i.e `TIMEOUT`, `PREEMPTED_BY_SCHEDULER` or `PLACEHOLDER_REPLACED`, we will need to trigger the pod delete. 
   
   Please let me know if this makes sense. Please read doc https://docs.google.com/document/d/1YpLZLqYsp5IaeEdl_AU9wZ-f16CfaaoRhZdalDP4RCg/, if there is anything not clear enough, feel free to comment.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (c4cafea) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.05%`.
   > The diff coverage is `85.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.43%   +0.05%     
   ==========================================
     Files          32       32              
     Lines        2825     2859      +34     
   ==========================================
   + Hits         1621     1642      +21     
   - Misses       1137     1147      +10     
   - Partials       67       70       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `60.46% <90.90%> (+10.46%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.95% <100.00%> (+0.81%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `66.97% <100.00%> (-4.52%)` | :arrow_down: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...c4cafea](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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



##########
File path: pkg/cache/application_events.go
##########
@@ -160,3 +162,34 @@ func (fe FailApplicationEvent) GetArgs() []interface{} {
 func (fe FailApplicationEvent) GetApplicationID() string {
 	return fe.applicationID
 }
+
+// ------------------------
+// Release application
+// ------------------------
+type ReleaseApplicationEvent struct {
+	applicationID  string
+	allocationUUID string
+	event          events.ApplicationEventType
+}

Review comment:
       can you pls add the TERMINATION_TYPE in this message as well?
   this gives us the flexibility in case we want to handle different types differently in the future

##########
File path: pkg/cache/application_events.go
##########
@@ -160,3 +162,34 @@ func (fe FailApplicationEvent) GetArgs() []interface{} {
 func (fe FailApplicationEvent) GetApplicationID() string {
 	return fe.applicationID
 }
+
+// ------------------------
+// Release application

Review comment:
       NIT: Release application allocations

##########
File path: pkg/cache/context.go
##########
@@ -552,6 +552,16 @@ func (ctx *Context) RemoveApplication(appID string) error {
 	}
 }
 
+func (ctx *Context) RemoveApplicationInternal(appID string) error {

Review comment:
       can you add a UT to cover this function?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #213: [YUNIKORN-478]Handle app completion at the shim side

Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #213:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/213


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

Posted by GitBox <gi...@apache.org>.
HuangTing-Yao edited a comment on pull request #213:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/213#issuecomment-749362028


   > hi @HuangTing-Yao any updates? If you have more questions, pls let me know. Thanks
   
   @yangwwei  almost done, I will upload new version today.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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



##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -118,15 +118,34 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 	for _, release := range response.ReleasedAllocations {
 		log.Logger().Debug("callback: response to released allocations",
 			zap.String("UUID", release.UUID))
+		// delete the allocated pod
+		app := callback.context.GetApplication(release.ApplicartionID)

Review comment:
       Need to check if app is nil or not, to avoid nil pointer exception

##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -118,15 +118,34 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 	for _, release := range response.ReleasedAllocations {
 		log.Logger().Debug("callback: response to released allocations",
 			zap.String("UUID", release.UUID))
+		// delete the allocated pod
+		app := callback.context.GetApplication(release.ApplicartionID)
+		for _, task := range app.taskMap {
+			if task.allocationUUID == release.UUID {
+				// TerminationType 0 mean STOPPED_BY_RM
+				if release.TerminationType != si.AllocationRelease_STOPPED_BY_RM {

Review comment:
       we need to handle this check before iterating the task map in order to make this more efficient




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (c4cafea) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.05%`.
   > The diff coverage is `85.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.43%   +0.05%     
   ==========================================
     Files          32       32              
     Lines        2825     2859      +34     
   ==========================================
   + Hits         1621     1642      +21     
   - Misses       1137     1147      +10     
   - Partials       67       70       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `60.46% <90.90%> (+10.46%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.95% <100.00%> (+0.81%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `66.97% <100.00%> (-4.52%)` | :arrow_down: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...bfb4fc0](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

Posted by GitBox <gi...@apache.org>.
HuangTing-Yao commented on pull request #213:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/213#issuecomment-749362028


   > hi @HuangTing-Yao any updates? If you have more questions, pls let me know. Thanks
   
   almost done, I will upload new version today.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (8ed8f1e) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **decrease** coverage by `0.32%`.
   > The diff coverage is `11.11%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   - Coverage   57.38%   57.05%   -0.33%     
   ==========================================
     Files          32       32              
     Lines        2825     2841      +16     
   ==========================================
     Hits         1621     1621              
   - Misses       1137     1153      +16     
     Partials       67       67              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.15% <0.00%> (-1.20%)` | :arrow_down: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.14% <0.00%> (-0.99%)` | :arrow_down: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `70.83% <0.00%> (-0.67%)` | :arrow_down: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...8ed8f1e](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (8ed8f1e) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **decrease** coverage by `0.32%`.
   > The diff coverage is `11.11%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   - Coverage   57.38%   57.05%   -0.33%     
   ==========================================
     Files          32       32              
     Lines        2825     2841      +16     
   ==========================================
     Hits         1621     1621              
   - Misses       1137     1153      +16     
     Partials       67       67              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.15% <0.00%> (-1.20%)` | :arrow_down: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.14% <0.00%> (-0.99%)` | :arrow_down: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `70.83% <0.00%> (-0.67%)` | :arrow_down: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...3d35b84](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   hi @HuangTing-Yao  there is some issue here. First, we need to update go.mod file in order to use the latest binary from scheduler-interface repo, where we get the new `UpdateResponse#AllocationRelease`. And one more thing I forgot to mention, from the doc:
   ```
    // Released allocations, this could be either ack from scheduler when RM asks to terminate some allocations.
     // Or it could be decision made by scheduler (such as preemption or timeout).
     repeated AllocationRelease releasedAllocations = 3;
   ```
   we need to handle this differently based on the different TERMINATION_TYPE, 
   
   ```
    enum TerminationType {
       STOPPED_BY_RM = 0;          // Stopped or killed by ResourceManager (created by RM)
       TIMEOUT = 1;                // Timed out based on the executionTimeoutMilliSeconds (created by core)
       PREEMPTED_BY_SCHEDULER = 2; // Preempted allocation by scheduler (created by core)
       PLACEHOLDER_REPLACED = 3;   // Placeholder allocation replaced by real allocation (created by core)
     }
   ```
   
   if `STOPPED_BY_RM`,  there is no operation needed in the shim side (today's behavior)
   if otherwise i.e `TIMEOUT`, `PREEMPTED_BY_SCHEDULER` or `PLACEHOLDER_REPLACED`, we will need to trigger the pod delete. 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=h1) Report
   > Merging [#213](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=desc) (7cca13a) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/fc51dbca25445aa2f7497a4a10a5b1b988481be0?el=desc) (fc51dbc) will **increase** coverage by `0.40%`.
   > The diff coverage is `85.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #213      +/-   ##
   ==========================================
   + Coverage   57.38%   57.78%   +0.40%     
   ==========================================
     Files          32       32              
     Lines        2825     2859      +34     
   ==========================================
   + Hits         1621     1652      +31     
   - Misses       1137     1138       +1     
   - Partials       67       69       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `79.04% <76.19%> (+0.69%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `60.46% <90.90%> (+10.46%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.95% <100.00%> (+0.81%)` | :arrow_up: |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `71.62% <100.00%> (+0.13%)` | :arrow_up: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `61.79% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=footer). Last update [fc51dbc...7cca13a](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei edited a comment on pull request #213: [YUNIKORN-478]Handle app completion at the shim side

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


   hi @HuangTing-Yao  there is some issue here. First, we need to update go.mod file in order to use the latest binary from scheduler-interface repo, where we get the new `UpdateResponse#AllocationRelease`. And one more thing I forgot to mention, from the doc:
   ```
    // Released allocations, this could be either ack from scheduler when RM asks to terminate some allocations.
     // Or it could be decision made by scheduler (such as preemption or timeout).
     repeated AllocationRelease releasedAllocations = 3;
   ```
   we need to handle this differently based on the different TERMINATION_TYPE, 
   
   ```
    enum TerminationType {
       STOPPED_BY_RM = 0;          // Stopped or killed by ResourceManager (created by RM)
       TIMEOUT = 1;                // Timed out based on the executionTimeoutMilliSeconds (created by core)
       PREEMPTED_BY_SCHEDULER = 2; // Preempted allocation by scheduler (created by core)
       PLACEHOLDER_REPLACED = 3;   // Placeholder allocation replaced by real allocation (created by core)
     }
   ```
   
   if `STOPPED_BY_RM`,  there is no operation needed in the shim side (today's behavior)
   if otherwise i.e `TIMEOUT`, `PREEMPTED_BY_SCHEDULER` or `PLACEHOLDER_REPLACED`, we will need to trigger the pod delete. 
   
   Please let me know if this makes sense. Please read doc https://docs.google.com/document/d/1YpLZLqYsp5IaeEdl_AU9wZ-f16CfaaoRhZdalDP4RCg/, if there is anything not clear enough, feel free to comment. 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.

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