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/08/12 05:20:37 UTC
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #277: YUNIKORN-641: Add events for placeholder timeout to pod
wilfred-s commented on a change in pull request #277:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/277#discussion_r687394861
##########
File path: pkg/cache/context.go
##########
@@ -583,7 +583,23 @@ func (ctx *Context) AddTask(request *interfaces.AddTaskRequest) interfaces.Manag
zap.String("appID", app.applicationID),
zap.String("taskID", task.taskID),
zap.String("taskState", task.GetTaskState()))
-
+ if app.requestOriginatingPod == nil {
+ for _, ownerReference := range app.placeholderOwnerReferences {
+ if task, taskErr := app.GetTask(string(ownerReference.UID)); taskErr == nil {
+ if task != nil {
+ log.Logger().Info("app request originating pod added",
+ zap.String("appID", app.applicationID),
+ zap.String("original pod", task.GetTaskPod().String()))
+ app.setRequestOriginatingPod(task.GetTaskPod())
+ break
+ }
+ }
+ }
+ } else {
+ log.Logger().Info("app request originating pod exist",
+ zap.String("appID", app.applicationID),
+ zap.String("original pod", app.requestOriginatingPod.String()))
Review comment:
Remove this we do not want to log this when we do not change anything. Could be 100's of pods just floods the log
##########
File path: pkg/cache/application.go
##########
@@ -58,6 +58,7 @@ type Application struct {
placeholderAsk *si.Resource // total placeholder request for the app (all task groups)
placeholderTimeoutInSec int64
schedulingStyle string
+ requestOriginatingPod *v1.Pod // Original Pod which creates the requests
Review comment:
Do I understand this correctly: this is a link that points to the pod of the first first task added into the application.
Why are we not using the task, either via the `taskID` which we lookup in the map or a task reference `*Task`?
##########
File path: pkg/cache/context.go
##########
@@ -583,7 +583,23 @@ func (ctx *Context) AddTask(request *interfaces.AddTaskRequest) interfaces.Manag
zap.String("appID", app.applicationID),
zap.String("taskID", task.taskID),
zap.String("taskState", task.GetTaskState()))
-
+ if app.requestOriginatingPod == nil {
+ for _, ownerReference := range app.placeholderOwnerReferences {
+ if task, taskErr := app.GetTask(string(ownerReference.UID)); taskErr == nil {
+ if task != nil {
+ log.Logger().Info("app request originating pod added",
+ zap.String("appID", app.applicationID),
+ zap.String("original pod", task.GetTaskPod().String()))
+ app.setRequestOriginatingPod(task.GetTaskPod())
+ break
Review comment:
Simplify this task != nil if taskErr == nil. Also see mention above about tracking the task not the pod.
--
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