[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #587: [YUNIKORN-1724] Improve the performance of shim side scheduling cycle

["craigcondit (via GitHub)" <gi...@apache.org>]

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


##########
pkg/cache/application.go:
##########
@@ -62,6 +63,68 @@ type Application struct {
 	placeholderTimeoutInSec    int64
 	schedulingStyle            string
 	originatingTask            interfaces.ManagedTask // Original Pod which creates the requests
+
+	newTasks *newTasksTracker
+}
+
+// For performance reasons, we track tasks with "New" state for quick retrieval.
+// Application.GetNewTasks (and getTasks() in general) must not be used on critical paths because it
+// uses a read lock inside fsm.FSM which becomes a bottleneck with large number of pods.
+type newTasksTracker struct {
+	sortedTasks *btree.BTree
+	sync.Mutex

Review Comment:
   Shouldn't we use an RWMutex here as reads are likely to (significantly) outstrip writes?



##########
pkg/cache/application_test.go:
##########
@@ -1343,3 +1343,64 @@ func (ctx *Context) addApplication(app *Application) {
 	defer ctx.lock.Unlock()
 	ctx.applications[app.applicationID] = app
 }
+
+func TestNewTaskTracker(t *testing.T) {
+	context := initContextForTest()
+	ms := newMockSchedulerAPI()
+	app := NewApplication(appID, "root.abc", "testuser", testGroups, map[string]string{}, ms)
+	tasks := []*Task{
+		NewTask("task01", app, context,
+			&v1.Pod{
+				ObjectMeta: apis.ObjectMeta{
+					CreationTimestamp: apis.Time{Time: time.Unix(100, 0)},
+				},
+			},
+		),
+		NewTask("task02", app, context,
+			&v1.Pod{
+				ObjectMeta: apis.ObjectMeta{
+					CreationTimestamp: apis.Time{Time: time.Unix(11, 0)},
+				},
+			},
+		),
+		NewTask("task03", app, context,
+			&v1.Pod{
+				ObjectMeta: apis.ObjectMeta{
+					CreationTimestamp: apis.Time{Time: time.Unix(22, 0)},
+				},
+			},
+		),
+		NewTask("task04", app, context,
+			&v1.Pod{
+				ObjectMeta: apis.ObjectMeta{
+					CreationTimestamp: apis.Time{Time: time.Unix(50, 0)},
+				},
+			},
+		),
+		NewTask("task05", app, context,
+			&v1.Pod{
+				ObjectMeta: apis.ObjectMeta{
+					CreationTimestamp: apis.Time{Time: time.Unix(1, 0)},
+				},
+			},
+		),
+	}
+
+	for _, task := range tasks {
+		app.addTask(task)
+	}
+	verifyNewTaskOrding(t, app)
+	app.removeTask("task04")
+	app.removeTask("task05")
+	verifyNewTaskOrding(t, app)
+	assert.Equal(t, 3, app.newTasks.count())
+}
+
+func verifyNewTaskOrding(t *testing.T, app *Application) {
+	newTasks := app.GetNewTasks()

Review Comment:
   I think app.GetNewTasks() should be refactored to use the new method. There's no point in leaving the old mechanism around. We can add an `app.HasNewTasks() bool` method as well for quickly determining if the list is empty.



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