You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/10/01 13:40:10 UTC
[GitHub] [incubator-yunikorn-core] adamantal opened a new pull request #211: [YUNIKORN-429] Set default logging level to INFO
adamantal opened a new pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211
Rationalized a few log items in the core:
- increased log level from DEBUG to INFO when:
- adding a new managed queue
- validating configs (once per partition). More granular logs are kept in DEBUG level
- registering scheduler plugins
- added a few extra lines in `entrypoint.go` to display what services have been started with YuniKorn
When the log levels are increased from DEBUG to INFO we will get rid of the following entries:
1. All the DEBUG level FTM log entries, like this:
```
2020-10-01T14:40:12.912+0200 DEBUG scheduler/scheduler.go:231 enqueued event {"eventType": "*schedulerevent.SchedulerUpdatePartitionsConfigEvent", "eventError": "json: unsupported type: chan *commonevents.Result", "currentQueueSize": 0}
```
I think we're fine to remove all these.
2. Outstanding request inspection:
```
2020-10-01T14:40:13.888+0200 DEBUG scheduler/scheduler.go:129 inspect outstanding requests
2020-10-01T14:42:24.923+0200 DEBUG scheduler/scheduler.go:135 outstanding request {"appID": "batch-sleep-job-1", "allocationKey": "3574688c-009b-452b-bdf8-2b66d499e02f"}
```
I think both of them can be kept in debug level, because in case of a busy cluster than can be lots of these logs entries, and by default this log appears in every second. I added an aggregated variation about the outstanding request in `inspectOutstandingRequests()` in INFO level. This gives the information about the size of the pending requests, but does not flood the logs.
3. Queue cleaner
```
2020-10-01T14:41:02.925+0200 DEBUG scheduler/partition_manager.go:62 time consumed for queue cleaner {"duration": "24.489µs"}
```
I think this can be kept in debug level as well, it is not an essential information.
4. Metrics collection
```
2020-10-01T14:41:12.887+0200 DEBUG metrics/metrics_collector.go:58 Adding current status to historical partition data
```
This log appears in every minute. I think if we display in `entrypoint.go` that YuniKorn have started the metrics collection there's no need to display this in every minute.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-702145174
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=h1) Report
> Merging [#211](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/1e23202cef2a9bc0798290135ab7570cc4d0a51e?el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `53.33%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #211 +/- ##
==========================================
- Coverage 60.82% 60.77% -0.06%
==========================================
Files 67 67
Lines 5688 5690 +2
==========================================
- Hits 3460 3458 -2
- Misses 2087 2091 +4
Partials 141 141
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsZXIuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/scheduling\_partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsaW5nX3BhcnRpdGlvbi5nbw==) | `40.28% <0.00%> (ø)` | |
| [pkg/cache/queue\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3F1ZXVlX2luZm8uZ28=) | `68.99% <50.00%> (-0.88%)` | :arrow_down: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `72.72% <60.00%> (ø)` | |
| [pkg/common/configs/config.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZy5nbw==) | `68.96% <100.00%> (ø)` | |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `86.87% <100.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `47.53% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=footer). Last update [29d6c82...f2b4556](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-702145085
Hey @adamantal,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/731939326">View build log</a>
###### TravisBuddy Request Identifier: 1dea4670-03ec-11eb-b614-dd2b96524a99
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-706209996
Hey @adamantal,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/734299247">View build log</a>
###### TravisBuddy Request Identifier: 77702030-0a3a-11eb-b8c5-47e0b209ea74
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-706184300
Hey @adamantal,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/734290545">View build log</a>
###### TravisBuddy Request Identifier: 44a36c30-0a34-11eb-b8c5-47e0b209ea74
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-702145174
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=h1) Report
> Merging [#211](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/74ca29415d848a80372cbd18b2bd76601ce2ff5e?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #211 +/- ##
==========================================
- Coverage 60.98% 60.98% -0.01%
==========================================
Files 67 67
Lines 5770 5774 +4
==========================================
+ Hits 3519 3521 +2
- Misses 2109 2111 +2
Partials 142 142
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `86.70% <ø> (-0.17%)` | :arrow_down: |
| [pkg/scheduler/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsZXIuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/scheduling\_partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsaW5nX3BhcnRpdGlvbi5nbw==) | `40.28% <0.00%> (ø)` | |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `72.72% <60.00%> (ø)` | |
| [pkg/cache/queue\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3F1ZXVlX2luZm8uZ28=) | `68.99% <100.00%> (ø)` | |
| [pkg/scheduler/scheduling\_application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsaW5nX2FwcGxpY2F0aW9uLmdv) | `71.06% <100.00%> (+0.32%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=footer). Last update [74ca294...f9ec6e4](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/211?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-702145174
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#discussion_r499924945
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -377,7 +377,7 @@ func Validate(newConfig *SchedulerConfig) error {
partition.Name = DefaultPartition
}
// check the queue structure
- log.Logger().Debug("checking partition",
+ log.Logger().Info("checking partition",
Review comment:
Do we need to include this in the INFO level, this line of log is not very informative won't be very much useful.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-706398769
Hi @adamantal Thanks for validating this, looks good. For the reservation/unreservation, we do a bunch of checks to make sure the reservation is only created on some valid nodes. You can try the following steps:
- submit a pod and get it running
- submit another pod that ask for more resources than what is available on the node, but less than the node total
- wait for a few seconds (we have a reservation-delay)
- check if reservation happens
for the changes in this PR, I think they look good enough. Pls double check the reservation-related logs, if there is any missing things, we can track in a different JIRA.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] adamantal commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
adamantal commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-706202211
Hi @yangwwei,
Used docker-desktop to validate the messages locally. I uploaded the INFO level logs of running some sleep jobs (a few of them were pending due to insufficient resources).
You can check too, that 1-3, 5-6 has traces in the logs.
One missing piece that I found is the reservation. There were pending pods in my scenario, but the reservation did not get triggered, so I could not see the related logs. Also I added an extra log in the reservation and the unreservation phase that may be helpful in the future.
I hope that this make sense. If you have any concerns please let me know.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei merged pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#issuecomment-706398923
Since this is only changing logs, code coverage failure won't be a concern. Merging this shortly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] adamantal commented on a change in pull request #211: [YUNIKORN-429] Set default logging level to INFO
Posted by GitBox <gi...@apache.org>.
adamantal commented on a change in pull request #211:
URL: https://github.com/apache/incubator-yunikorn-core/pull/211#discussion_r502429168
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -377,7 +377,7 @@ func Validate(newConfig *SchedulerConfig) error {
partition.Name = DefaultPartition
}
// check the queue structure
- log.Logger().Debug("checking partition",
+ log.Logger().Info("checking partition",
Review comment:
Remove the unnecessary log.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org