[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #418: [POC][YUNIKORN-1186][YUNIKORN-1194] Race condition during recovery

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

craigcondit commented on code in PR #418:
URL: https://github.com/apache/yunikorn-k8shim/pull/418#discussion_r877289328


##########
pkg/appmgmt/interfaces/recoverable.go:
##########
@@ -35,7 +35,7 @@ import (
 // and then properly recover these applications before recovering nodes.
 type Recoverable interface {
 	// list applications returns all existing applications known to this app manager.
-	ListApplications() (map[string]ApplicationMetadata, error)
+	ListApplications() ([]*v1.Pod, error)

Review Comment:
   Let's call this ListPods() or something since apps don't have anything to do with it any more.



##########
pkg/appmgmt/general/podevent_handler.go:
##########
@@ -0,0 +1,176 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package general
+
+import (
+	"sync"
+
+	"github.com/apache/yunikorn-k8shim/pkg/appmgmt/interfaces"
+	"github.com/apache/yunikorn-k8shim/pkg/log"
+
+	"go.uber.org/zap"
+
+	v1 "k8s.io/api/core/v1"
+)
+
+type PodEventHandler struct {
+	recoveryRunning bool
+	amProtocol      interfaces.ApplicationManagementProtocol
+	asyncEvents     []*podAsyncEvent
+	sync.Mutex
+}
+
+const (
+	AddPod = iota
+	UpdatePod
+	DeletePod
+)
+
+const (
+	Recovery = iota
+	Informers
+)
+
+type EventType int
+type EventSource int
+
+type podAsyncEvent struct {
+	eventType EventType
+	pod       *v1.Pod
+}
+
+func (p *PodEventHandler) HandleEvent(eventType EventType, source EventSource, pod *v1.Pod) interfaces.ManagedApp {

Review Comment:
   Can we extract everything within the locks into a separate method that does locking using defer? that method could return a bool indicating whether the event has been processed. Something like:
   
   ```
   func (p *PodEventHandler) HandleEvent(eventType EventType, source EventSource, pod *v1.Pod) interfaces.ManagedApp {
   	if p.handleRecoveryEvent(eventType, source, pod) {
   		return nil
   	}
   	return p.internalHandle(eventType, source, pod)
   }
   
   
   func (p *PodEventHandler) handleRecoveryEvent(eventType EventType, source EventSource, pod *v1.Pod) bool {
   	p.Lock()
   	defer p.Unlock()
   
   	if p.recoveryRunning && source == Informers {
   		log.Logger().Debug("Storing async event", zap.Int("eventType", int(eventType)),
   			zap.String("pod", pod.GetName()))
   		p.asyncEvents = append(p.asyncEvents, &podAsyncEvent{eventType, pod})
   		return true
   	}
   	return false
   }
   ```



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