Skip to content

Partially revert the event discard change in #2748.#2770

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:partial-revert-#2748
Nov 9, 2018
Merged

Partially revert the event discard change in #2748.#2770
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:partial-revert-#2748

Conversation

@Random-Liu
Copy link
Member

Per discussion at #2762 (comment), we decided not to discard events, because it breaks the assumption that client can rely on TaskExit events to detect container stop.

If TaskExit events can be discarded, the client has to do periodically state check, which is not ideal.

Signed-off-by: Lantao Liu lantaol@google.com

Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu Random-Liu added this to the 1.2.1 milestone Nov 8, 2018
@Random-Liu
Copy link
Member Author

Random-Liu commented Nov 8, 2018

I found that #2748 is only in master branch.

We may want #2765, #2748 and this change in 1.2.

@estesp
Copy link
Member

estesp commented Nov 8, 2018

Took me a second, but now I get what you are saying--we have branched release/1.2 so now have to consider it for cherry-picks/bug fixes.

@codecov-io
Copy link

Codecov Report

Merging #2770 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2770   +/-   ##
=======================================
  Coverage   43.72%   43.72%           
=======================================
  Files         100      100           
  Lines       10734    10734           
=======================================
  Hits         4693     4693           
  Misses       5311     5311           
  Partials      730      730
Flag Coverage Δ
#linux 47.39% <ø> (ø) ⬆️
#windows 40.89% <ø> (ø) ⬆️

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 130d07e...c524b9c. Read the comment docs.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Member

LGTM

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.

4 participants