You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "pbacsko (via GitHub)" <gi...@apache.org> on 2024/04/30 23:06:53 UTC
[PR] [YUNIKORN-2597] Improve error messages in Context [yunikorn-k8shim]
pbacsko opened a new pull request, #829:
URL: https://github.com/apache/yunikorn-k8shim/pull/829
### What is this PR for?
Improve event handler related logging for `cache.Context`.
### What type of PR is it?
* [ ] - Bug Fix
* [x] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2597
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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
Re: [PR] [YUNIKORN-2597] Improve error messages in Context [yunikorn-k8shim]
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko closed pull request #829: [YUNIKORN-2597] Improve error messages in Context
URL: https://github.com/apache/yunikorn-k8shim/pull/829
--
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
Re: [PR] [YUNIKORN-2597] Improve error messages in Context [yunikorn-k8shim]
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #829:
URL: https://github.com/apache/yunikorn-k8shim/pull/829#discussion_r1586050862
##########
pkg/cache/context.go:
##########
@@ -1294,41 +1295,68 @@ func (ctx *Context) HandleContainerStateUpdate(request *si.UpdateContainerSchedu
func (ctx *Context) ApplicationEventHandler() func(obj interface{}) {
return func(obj interface{}) {
if event, ok := obj.(events.ApplicationEvent); ok {
- app := ctx.GetApplication(event.GetApplicationID())
+ appID := event.GetApplicationID()
+ app := ctx.GetApplication(appID)
if app == nil {
- log.Log(log.ShimContext).Error("failed to handle application event",
- zap.String("reason", "application not exist"))
+ log.Log(log.ShimContext).Error("failed to handle application event, application does not exist",
+ zap.String("applicationID", appID))
return
}
+
if app.canHandle(event) {
if err := app.handle(event); err != nil {
log.Log(log.ShimContext).Error("failed to handle application event",
zap.String("event", event.GetEvent()),
+ zap.String("applicationID", appID),
zap.Error(err))
+ return
}
}
+ log.Log(log.ShimContext).Error("application event cannot be handled in the current state",
Review Comment:
Correct, I modified that. Initially it was a bunch of if-else which was hard to read, I do hope this is better.
##########
pkg/cache/context.go:
##########
@@ -1294,41 +1295,68 @@ func (ctx *Context) HandleContainerStateUpdate(request *si.UpdateContainerSchedu
func (ctx *Context) ApplicationEventHandler() func(obj interface{}) {
return func(obj interface{}) {
if event, ok := obj.(events.ApplicationEvent); ok {
- app := ctx.GetApplication(event.GetApplicationID())
+ appID := event.GetApplicationID()
+ app := ctx.GetApplication(appID)
if app == nil {
- log.Log(log.ShimContext).Error("failed to handle application event",
- zap.String("reason", "application not exist"))
+ log.Log(log.ShimContext).Error("failed to handle application event, application does not exist",
+ zap.String("applicationID", appID))
return
}
+
if app.canHandle(event) {
if err := app.handle(event); err != nil {
log.Log(log.ShimContext).Error("failed to handle application event",
zap.String("event", event.GetEvent()),
+ zap.String("applicationID", appID),
zap.Error(err))
+ return
}
}
+ log.Log(log.ShimContext).Error("application event cannot be handled in the current state",
+ zap.String("applicationID", appID),
+ zap.String("event", event.GetEvent()),
+ zap.String("state", app.sm.Current()))
+ return
}
+
+ log.Log(log.ShimContext).Error("could not handle application event",
+ zap.String("type", reflect.TypeOf(obj).Name()))
}
}
func (ctx *Context) TaskEventHandler() func(obj interface{}) {
return func(obj interface{}) {
if event, ok := obj.(events.TaskEvent); ok {
- task := ctx.getTask(event.GetApplicationID(), event.GetTaskID())
+ appID := event.GetApplicationID()
+ taskID := event.GetTaskID()
+ task := ctx.getTask(appID, taskID)
if task == nil {
- log.Log(log.ShimContext).Error("failed to handle application event")
+ log.Log(log.ShimContext).Error("failed to handle task event, task does not exist",
+ zap.String("applicationID", appID),
+ zap.String("taskID", taskID))
return
}
+
if task.canHandle(event) {
if err := task.handle(event); err != nil {
log.Log(log.ShimContext).Error("failed to handle task event",
- zap.String("applicationID", task.applicationID),
- zap.String("taskID", task.taskID),
+ zap.String("applicationID", appID),
+ zap.String("taskID", taskID),
zap.String("event", event.GetEvent()),
zap.Error(err))
+ return
}
}
+ log.Log(log.ShimContext).Error("task event cannot be handled in the current state",
Review Comment:
Fixed.
--
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
Re: [PR] [YUNIKORN-2597] Improve error messages in Context [yunikorn-k8shim]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #829:
URL: https://github.com/apache/yunikorn-k8shim/pull/829#issuecomment-2087693659
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/829?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `53.33333%` with `14 lines` in your changes are missing coverage. Please review.
> Project coverage is 66.06%. Comparing base [(`a1b8480`)](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/a1b8480d9a2b5580bbfdbfccbc2631da343937c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`d2420f5`)](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/829?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/829?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [pkg/cache/context.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/829?src=pr&el=tree&filepath=pkg%2Fcache%2Fcontext.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | 53.33% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/829?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #829 +/- ##
==========================================
- Coverage 66.10% 66.06% -0.05%
==========================================
Files 69 69
Lines 7518 7541 +23
==========================================
+ Hits 4970 4982 +12
- Misses 2337 2348 +11
Partials 211 211
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/829?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] [YUNIKORN-2597] Improve error messages in Context [yunikorn-k8shim]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #829:
URL: https://github.com/apache/yunikorn-k8shim/pull/829#discussion_r1585862502
##########
pkg/cache/context.go:
##########
@@ -1294,41 +1295,68 @@ func (ctx *Context) HandleContainerStateUpdate(request *si.UpdateContainerSchedu
func (ctx *Context) ApplicationEventHandler() func(obj interface{}) {
return func(obj interface{}) {
if event, ok := obj.(events.ApplicationEvent); ok {
- app := ctx.GetApplication(event.GetApplicationID())
+ appID := event.GetApplicationID()
+ app := ctx.GetApplication(appID)
if app == nil {
- log.Log(log.ShimContext).Error("failed to handle application event",
- zap.String("reason", "application not exist"))
+ log.Log(log.ShimContext).Error("failed to handle application event, application does not exist",
+ zap.String("applicationID", appID))
return
}
+
if app.canHandle(event) {
if err := app.handle(event); err != nil {
log.Log(log.ShimContext).Error("failed to handle application event",
zap.String("event", event.GetEvent()),
+ zap.String("applicationID", appID),
zap.Error(err))
+ return
}
}
+ log.Log(log.ShimContext).Error("application event cannot be handled in the current state",
+ zap.String("applicationID", appID),
+ zap.String("event", event.GetEvent()),
+ zap.String("state", app.sm.Current()))
+ return
}
+
+ log.Log(log.ShimContext).Error("could not handle application event",
+ zap.String("type", reflect.TypeOf(obj).Name()))
}
}
func (ctx *Context) TaskEventHandler() func(obj interface{}) {
return func(obj interface{}) {
if event, ok := obj.(events.TaskEvent); ok {
- task := ctx.getTask(event.GetApplicationID(), event.GetTaskID())
+ appID := event.GetApplicationID()
+ taskID := event.GetTaskID()
+ task := ctx.getTask(appID, taskID)
if task == nil {
- log.Log(log.ShimContext).Error("failed to handle application event")
+ log.Log(log.ShimContext).Error("failed to handle task event, task does not exist",
+ zap.String("applicationID", appID),
+ zap.String("taskID", taskID))
return
}
+
if task.canHandle(event) {
if err := task.handle(event); err != nil {
log.Log(log.ShimContext).Error("failed to handle task event",
- zap.String("applicationID", task.applicationID),
- zap.String("taskID", task.taskID),
+ zap.String("applicationID", appID),
+ zap.String("taskID", taskID),
zap.String("event", event.GetEvent()),
zap.Error(err))
+ return
}
}
+ log.Log(log.ShimContext).Error("task event cannot be handled in the current state",
Review Comment:
ditto
##########
pkg/cache/context.go:
##########
@@ -1294,41 +1295,68 @@ func (ctx *Context) HandleContainerStateUpdate(request *si.UpdateContainerSchedu
func (ctx *Context) ApplicationEventHandler() func(obj interface{}) {
return func(obj interface{}) {
if event, ok := obj.(events.ApplicationEvent); ok {
- app := ctx.GetApplication(event.GetApplicationID())
+ appID := event.GetApplicationID()
+ app := ctx.GetApplication(appID)
if app == nil {
- log.Log(log.ShimContext).Error("failed to handle application event",
- zap.String("reason", "application not exist"))
+ log.Log(log.ShimContext).Error("failed to handle application event, application does not exist",
+ zap.String("applicationID", appID))
return
}
+
if app.canHandle(event) {
if err := app.handle(event); err != nil {
log.Log(log.ShimContext).Error("failed to handle application event",
zap.String("event", event.GetEvent()),
+ zap.String("applicationID", appID),
zap.Error(err))
+ return
}
}
+ log.Log(log.ShimContext).Error("application event cannot be handled in the current state",
Review Comment:
This log message is weird to me since the application could be get handled successfully rather than "cannot be handled" in this path. Maybe we need to add return to `if app.canHandle(event) {`
--
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