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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
