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