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



##########
File path: pkg/rmproxy/rmevent/events.go
##########
@@ -88,6 +88,12 @@ type RMReleaseAllocationEvent struct {
 	ReleasedAllocations []*si.AllocationRelease
 }
 
+type RMReleaseAllocationSyncEvent struct {
+	RmID                string
+	ReleasedAllocations []*si.AllocationRelease
+	Channel             chan *Result
+}
+

Review comment:
       Can we hide this in the logic? Have only one event (not a sync and async version).
   For a sync event you add a channel in the caller and wait for the response on the channel. For the async processing we can pass in a nil channel. Make the proxy handle it. I think it will allow us to remove code duplication.

##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -98,13 +98,21 @@ func (rmp *RMProxy) processAllocationUpdateEvent(event *rmevent.RMNewAllocations
 	rmp.RLock()
 	defer rmp.RUnlock()
 	if len(event.Allocations) == 0 {
+		event.Channel <- &rmevent.Result{
+			Reason:    "RMNewAllocationsEvent has zero allocations"	,
+			Succeeded: false,
+		}

Review comment:
       an empty list of allocations should not be a failure that changes the current behaviour we have now




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