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-641]: Add events for placeholder timeout to pod #277

Closed
wants to merge 9 commits into from

Conversation

manirajv06
Copy link
Contributor

What is this PR for?

Pod originates the app request has been considered as "first pod" to send event incase of placeholders timeout. This "first pod" is derived using pod's owner references as it already contains the necessary info once the corresponding task has been added to the app.

What type of PR is it?

  • - Improvement

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-641

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #277 (6534201) into master (1d650e7) will increase coverage by 0.57%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
+ Coverage   65.53%   66.10%   +0.57%     
==========================================
  Files          40       40              
  Lines        6406     6441      +35     
==========================================
+ Hits         4198     4258      +60     
+ Misses       2045     2018      -27     
- Partials      163      165       +2     
Impacted Files Coverage Δ
pkg/appmgmt/general/general.go 72.76% <50.00%> (ø)
pkg/cache/application.go 73.97% <96.15%> (+0.92%) ⬆️
pkg/cache/amprotocol_mock.go 43.54% <100.00%> (+43.54%) ⬆️
pkg/cache/context.go 46.30% <100.00%> (+0.44%) ⬆️
pkg/cache/placeholder.go 92.95% <100.00%> (ø)
pkg/cache/placeholder_manager.go 92.10% <0.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d650e7...6534201. Read the comment docs.

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.

overall approach is good, minor points on what to track and logs

pkg/cache/application.go Outdated Show resolved Hide resolved
pkg/cache/context.go Outdated Show resolved Hide resolved
pkg/cache/context.go Outdated Show resolved Hide resolved
@yangwwei yangwwei changed the title YUNIKORN-641: Add events for placeholder timeout to pod [YUNIKORN-641]: Add events for placeholder timeout to pod Aug 17, 2021
@manirajv06
Copy link
Contributor Author

@craigcondit Can you please take a look?

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

Looks reasonable, there's a couple minor things to address.

pkg/appmgmt/general/general.go Show resolved Hide resolved
pkg/cache/application.go Show resolved Hide resolved
pkg/cache/application.go Outdated Show resolved Hide resolved
pkg/cache/application.go Outdated Show resolved Hide resolved
pkg/cache/context.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

Found some minor stuff

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

+1 LGTM

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

+1 will merge shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants