[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #230: [YUNIKORN-484] Added waiting state timeout

[GitBox <gi...@apache.org>]

wilfred-s commented on a change in pull request #230:
URL: https://github.com/apache/incubator-yunikorn-core/pull/230#discussion_r553098103



##########
File path: pkg/scheduler/partition.go
##########
@@ -919,6 +921,9 @@ func (pc *PartitionContext) GetApplications() []*objects.Application {
 	for _, app := range pc.applications {
 		appList = append(appList, app)
 	}
+	for _, app := range pc.completedApplications {
+		appList = append(appList, app)
+	}

Review comment:
       This will look strange in the web UI. The completed applications will now be shown as part of the queues.
   If we keep old apps for a long period of time this will have a huge impact on the web UI performance.
   I think we need to split this up.

##########
File path: pkg/scheduler/partition.go
##########
@@ -1112,3 +1117,16 @@ func (pc *PartitionContext) removeAllocationAsk(appID string, allocationKey stri
 		}
 	}
 }
+
+// Move all the completed apps into the completedApp list
+// Delete all the applications marked for removal
+func (pc *PartitionContext) cleanupApps() {
+	for _, app := range pc.GetApplications() {

Review comment:
       I would expect that this works directly on the applications list and does a range over the apps moving them into the completed map:
   ```
   pc.Lock
   defer pc.Unlock
   for app := range pc.applications {
     if app.IsCompleted() {
       delete(pc.applications, app)
       pc.completedApplications[app.appID] = app
   }
   ```
   The completedApplication cleanup can be run separately at a far lower interval.

##########
File path: pkg/scheduler/partition.go
##########
@@ -1112,3 +1117,16 @@ func (pc *PartitionContext) removeAllocationAsk(appID string, allocationKey stri
 		}
 	}
 }
+
+// Move all the completed apps into the completedApp list
+// Delete all the applications marked for removal
+func (pc *PartitionContext) cleanupApps() {
+	for _, app := range pc.GetApplications() {
+		if app.IsCompleted() {
+			pc.removeApplication(app.ApplicationID)

Review comment:
       An application can only be `completed` if there is nothing left running reserved etc as it comes after `waiting`.
   Not sure why we would need to call `removeApplication` on it.
   I would expect that the state change of the app from waiting to completed removes the app from the queue, sets the queue link in the app to `nil` and just leaves the queue name.




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