[GitHub] [incubator-yunikorn-core] pbacsko commented on a change in pull request #353: YUNIKORN-985: Enforcing MaxRunningApps limit

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

pbacsko commented on a change in pull request #353:
URL: https://github.com/apache/incubator-yunikorn-core/pull/353#discussion_r778729546



##########
File path: pkg/scheduler/usergroupmanagement/user.go
##########
@@ -0,0 +1,57 @@
+package usergroupmanagement
+
+import (
+	"sync/atomic"
+
+	"github.com/apache/incubator-yunikorn-core/pkg/common/resources"
+)
+
+type User struct {
+	name	string // Name of the user
+	maxResources	*resources.Resource // Max Resource configured per user
+	maxApplications	int32 // Max Applications configured per user
+	runningApplications	*int32 // Running Applications
+	usedGroup	string
+}
+
+func NewUser(user string) *User {
+	return &User{
+		name:	user,
+	}
+}
+
+func (u *User) GetName() string {
+	return u.name
+}
+
+func (u *User) SetMaxApplications(maxApplications int32) {
+	u.maxApplications = maxApplications
+}
+
+func (u *User) IncRunningApplications() {
+	atomic.AddInt32(u.runningApplications, 1)
+}
+
+func (u *User) DecRunningApplications() {
+	atomic.AddInt32(u.runningApplications, -1)
+}
+
+func (u *User) CanRun() bool {
+	if atomic.LoadInt32(u.runningApplications) < u.maxApplications {

Review comment:
       This part should be double-checked, because it feels racy. 
   
   By the time `LoadInt()` completes, a separate goroutine might come along and increase `runningApplications`. This feels more sound:
   
   ```
   	current := atomic.LoadInt32(&runningApplications)
   	if atomic.CompareAndSwapInt32(&runningApplications, current, current) < u.maxApplications {
   		// runningApplications was definitely not incremented if we end up here
   	}
   ```
   
   It's easy to get this wrong. Also, I don't think we deliver to 32-bit targets, so I'd recommend `int64` instead.
   
   If performance is not a concern (and this does not look like a hot code path), then an ordinary `Mutex` is probably better because it's easiser to reason about. Atomic stuff is too low level.

##########
File path: pkg/scheduler/usergroupmanagement/user.go
##########
@@ -0,0 +1,57 @@
+package usergroupmanagement
+
+import (
+	"sync/atomic"
+
+	"github.com/apache/incubator-yunikorn-core/pkg/common/resources"
+)
+
+type User struct {
+	name	string // Name of the user
+	maxResources	*resources.Resource // Max Resource configured per user
+	maxApplications	int32 // Max Applications configured per user
+	runningApplications	*int32 // Running Applications
+	usedGroup	string
+}
+
+func NewUser(user string) *User {
+	return &User{
+		name:	user,
+	}
+}
+
+func (u *User) GetName() string {
+	return u.name
+}
+
+func (u *User) SetMaxApplications(maxApplications int32) {
+	u.maxApplications = maxApplications
+}
+
+func (u *User) IncRunningApplications() {
+	atomic.AddInt32(u.runningApplications, 1)
+}
+
+func (u *User) DecRunningApplications() {
+	atomic.AddInt32(u.runningApplications, -1)
+}
+
+func (u *User) CanRun() bool {
+	if atomic.LoadInt32(u.runningApplications) < u.maxApplications {

Review comment:
       This part should be double-checked, because it feels racy. 
   
   By the time `LoadInt32()` completes, a separate goroutine might come along and increase `runningApplications`. This feels more sound:
   
   ```
   	current := atomic.LoadInt32(&runningApplications)
   	if atomic.CompareAndSwapInt32(&runningApplications, current, current) < u.maxApplications {
   		// runningApplications was definitely not incremented if we end up here
   	}
   ```
   
   It's easy to get this wrong. Also, I don't think we deliver to 32-bit targets, so I'd recommend `int64` instead.
   
   If performance is not a concern (and this does not look like a hot code path), then an ordinary `Mutex` is probably better because it's easiser to reason about. Atomic stuff is too low level.




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