[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change

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

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



##########
File path: pkg/scheduler/context.go
##########
@@ -840,6 +797,7 @@ func (cc *ClusterContext) notifyRMAllocationReleased(rmID string, released []*ob
 	releaseEvent := &rmevent.RMReleaseAllocationEvent{
 		ReleasedAllocations: make([]*si.AllocationRelease, 0),
 		RmID:                rmID,
+		Channel:             nil,

Review comment:
       This should no longer be the case based on the SI discussions.

##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -97,14 +97,15 @@ func (rmp *RMProxy) handleUpdateResponseError(rmID string, err error) {
 func (rmp *RMProxy) processAllocationUpdateEvent(event *rmevent.RMNewAllocationsEvent) {
 	rmp.RLock()
 	defer rmp.RUnlock()
-	if len(event.Allocations) == 0 {
-		return
-	}

Review comment:
       I think we can leave this optimisation in the code: if no allocations are given we should return the success immediately otherwise process:
   ```
   if len(event.Allocations) != 0 {
   	// do what we need to do
   }
   // notify the channel
   ```

##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -139,14 +140,18 @@ func (rmp *RMProxy) processApplicationUpdateEvent(event *rmevent.RMApplicationUp
 func (rmp *RMProxy) processRMReleaseAllocationEvent(event *rmevent.RMReleaseAllocationEvent) {
 	rmp.RLock()
 	defer rmp.RUnlock()
-	if len(event.ReleasedAllocations) == 0 {
-		return
-	}
 	response := &si.AllocationResponse{
 		Released: event.ReleasedAllocations,
 	}
 	rmp.triggerUpdateAllocation(event.RmID, response)
 	metrics.GetSchedulerMetrics().AddReleasedContainers(len(event.ReleasedAllocations))
+
+	// Done, notify channel
+	if event.Channel != nil {

Review comment:
       Is it still the case that the channel can be nil after the updates we have discussed?

##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -139,14 +140,18 @@ func (rmp *RMProxy) processApplicationUpdateEvent(event *rmevent.RMApplicationUp
 func (rmp *RMProxy) processRMReleaseAllocationEvent(event *rmevent.RMReleaseAllocationEvent) {
 	rmp.RLock()
 	defer rmp.RUnlock()
-	if len(event.ReleasedAllocations) == 0 {
-		return
-	}

Review comment:
       Same as above use the optimisation




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