chia7712 commented on code in PR #810:
URL: https://github.com/apache/yunikorn-core/pull/810#discussion_r1545245961
##########
pkg/scheduler/ugm/queue_tracker.go:
##########
@@ -122,11 +120,9 @@ func (qt *QueueTracker) increaseTrackedResource(hierarchy
[]string, applicationI
zap.Bool("use wild card", qt.useWildCard),
zap.Stringer("total resource after increasing",
qt.resourceUsage),
zap.Int("total applications after increasing",
len(qt.runningApplications)))
- return true
}
-// Note: Lock free call. The Lock of the linked tracker (UserTracker and
GroupTracker) should be held before calling this function.
Review Comment:
it seems to me this comment is still legal. why remove it?
##########
pkg/scheduler/ugm/queue_tracker.go:
##########
@@ -140,12 +136,9 @@ func (qt *QueueTracker) decreaseTrackedResource(hierarchy
[]string, applicationI
if qt.childQueueTrackers[childName] == nil {
log.Log(log.SchedUGM).Error("Child queueTracker tracker
must be available in child queues map",
zap.String("child queueTracker name",
childName))
- return false, false
- }
- removeQT, decreased :=
qt.childQueueTrackers[childName].decreaseTrackedResource(hierarchy[1:],
applicationID, usage, removeApp)
- if !decreased {
- return false, decreased
+ return false
}
+ removeQT :=
qt.childQueueTrackers[childName].decreaseTrackedResource(hierarchy[1:],
applicationID, usage, removeApp)
Review Comment:
ditto
```go
if
qt.childQueueTrackers[childName].decreaseTrackedResource(hierarchy[1:],
applicationID, usage, removeApp) {
log.Log(log.SchedUGM).Debug("Removed queue tracker
linkage from its parent",
zap.String("queue path ", qt.queuePath),
zap.String("removed queue name", childName),
zap.String("parent queue name", qt.queueName))
delete(qt.childQueueTrackers, childName)
}
```
##########
pkg/scheduler/ugm/manager.go:
##########
@@ -132,45 +132,37 @@ func (m *Manager) DecreaseTrackedResource(queuePath,
applicationID string, usage
zap.String("tracked group", appGroup),
zap.Stringer("resource", usage),
zap.Bool("removeApp", removeApp))
- removeQT, decreased := userTracker.decreaseTrackedResource(queuePath,
applicationID, usage, removeApp)
- if !decreased {
- return decreased
- }
+ removeQT := userTracker.decreaseTrackedResource(queuePath,
applicationID, usage, removeApp)
if removeQT {
log.Log(log.SchedUGM).Info("Removing user from manager",
zap.String("user", user.User))
delete(m.userTrackers, user.User)
}
// if the app did not have a group we're done otherwise update the
groupTracker
if appGroup == common.Empty {
- return decreased
+ return
}
groupTracker := m.GetGroupTracker(appGroup)
if groupTracker == nil {
log.Log(log.SchedUGM).Error("group tracker should be available
in groupTrackers map",
zap.String("applicationID", applicationID),
zap.String("applicationID", appGroup))
- return decreased
+ return
}
log.Log(log.SchedUGM).Debug("Decreasing resource usage for group",
zap.String("group", appGroup),
zap.String("queue path", queuePath),
zap.String("application", applicationID),
zap.Stringer("resource", usage),
zap.Bool("removeApp", removeApp))
- removeQT, decreased = groupTracker.decreaseTrackedResource(queuePath,
applicationID, usage, removeApp)
- if !decreased {
- return decreased
- }
- if removeQT {
+ if removeGT := groupTracker.decreaseTrackedResource(queuePath,
applicationID, usage, removeApp); removeGT {
Review Comment:
how about
```go
if groupTracker.decreaseTrackedResource(queuePath, applicationID,
usage, removeApp) {
log.Log(log.SchedUGM).Info("Removing group from manager",
zap.String("group", appGroup),
zap.String("queue path", queuePath),
zap.String("application", applicationID),
zap.Bool("removeApp", removeApp))
delete(m.groupTrackers, appGroup)
}
```
--
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]
