Skip to content
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-25] Logging improvements #124

Closed
wants to merge 14 commits into from
Closed

[YUNIKORN-25] Logging improvements #124

wants to merge 14 commits into from

Conversation

yangwwei
Copy link
Contributor

@yangwwei yangwwei commented Apr 5, 2020

Improve logging messages

@yangwwei yangwwei self-assigned this Apr 5, 2020
@yangwwei
Copy link
Contributor Author

yangwwei commented Apr 5, 2020

hi @wilfred-s
could you please help to review the PR?
The purpose to keep essential logging at INFO level, and remove some of the useless logs.

Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some changes back to allow progress tracking
some code changes

@@ -562,7 +562,14 @@ func (m *ClusterInfo) processAllocationProposalEvent(event *cacheevent.Allocatio
RejectedAllocations: event.AllocationProposals[:1],
})
return
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need for an else clause.
logging should be outside

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. Done.

pkg/common/configs/configvalidator.go Outdated Show resolved Hide resolved
pkg/common/configs/configvalidator.go Outdated Show resolved Hide resolved
pkg/common/configs/configvalidator.go Outdated Show resolved Hide resolved
pkg/scheduler/partition_manager.go Outdated Show resolved Hide resolved
@@ -109,7 +106,8 @@ func (manager partitionManager) cleanQueues(schedulingQueue *SchedulingQueue) {
}
} else {
// TODO time out waiting for draining and removal
log.Logger().Debug("failed to remove scheduling queue due to existing assigned apps or leaf queues",
log.Logger().Debug("skip removing the scheduling queue",
zap.String("reason", "there are existing assigned apps or leaf queues"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have not used this setup anywhere splitting message into two, not sure about this but don't mind it either

Comment on lines 579 to 583
if alloc != nil {
log.Logger().Debug("try to allocate resources for a request from a reservation",
zap.String("allocation", alloc.String()))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already logged in the scheduling_queue.tryReservedAllocate()

pkg/scheduler/scheduling_application.go Outdated Show resolved Hide resolved
@wilfred-s wilfred-s closed this in baf42fd Apr 15, 2020
@wilfred-s wilfred-s deleted the YUNIKORN-25 branch April 15, 2020 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants