-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[YUNIKORN-2683] Unnecessary error is logged when resource usage is increased #896
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #896 +/- ##
==========================================
+ Coverage 78.01% 78.26% +0.24%
==========================================
Files 97 97
Lines 12119 12210 +91
==========================================
+ Hits 9455 9556 +101
+ Misses 2354 2349 -5
+ Partials 310 305 -5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbacsko overall +1 except for one small question.
@@ -107,9 +107,6 @@ func (m *Manager) IncreaseTrackedResource(queuePath, applicationID string, usage | |||
zap.Stringer("resource", usage)) | |||
groupTracker := m.GetGroupTracker(appGroup) | |||
if groupTracker == nil { | |||
log.Log(log.SchedUGM).Error("group tracker should be available in groupTrackers map", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we align the behavior of DecreaseTrackedResource
[0] with this change?
[0]
yunikorn-core/pkg/scheduler/ugm/manager.go
Line 110 in 97c38a9
log.Log(log.SchedUGM).Error("group tracker should be available in groupTrackers map", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case we're guarded by:
if appGroup == common.Empty {
return
}
In this code path, we already check it inside ensureGroupTrackerForApp()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is interesting. We do the similar check/ensure for IncreaseTrackedResource
m.ensureGroupTrackerForApp(queuePath, applicationID, user) |
It seems the error message makes sense since groupTracker
should be existent in the code path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be there. Before the refactor, we did not have this check because you can call IncreaseTrackedResource()
on a nil
group tracker. However I don't like that approach, just want to return on nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for one more question:
In that case we're guarded by:
why we not adding similar code to IncreaseTrackedResource
for consistency? It means that is fine to return directly if appGroup
is empty. Also, we add error log if appGroup
is NOT empty and ensureGroupTrackerForApp
is called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the latest version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbacsko thanks for updated PR!
pkg/scheduler/ugm/manager.go
Outdated
@@ -99,6 +99,9 @@ func (m *Manager) IncreaseTrackedResource(queuePath, applicationID string, usage | |||
} | |||
userTracker.increaseTrackedResource(queuePath, applicationID, usage) | |||
appGroup := userTracker.getGroupForApp(applicationID) | |||
if appGroup == common.Empty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this check should happen after log "Increasing resource usage for user"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is this PR for?
Don't log an error when there's no group tracker, this can be a perfectly normal situation.
The problem was caused by a refactor done in YUNIKORN-2542 (#876).
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2683
How should this be tested?
Screenshots (if appropriate)
Questions: