Skip to content

[orchestrator/task reaper] Clean up tasks in dirty list for which the service has been deleted#2666

Merged
cyli merged 1 commit into
moby:masterfrom
anshulpundir:reaper
Jun 19, 2018
Merged

[orchestrator/task reaper] Clean up tasks in dirty list for which the service has been deleted#2666
cyli merged 1 commit into
moby:masterfrom
anshulpundir:reaper

Conversation

@anshulpundir
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@dani-docker dani-docker left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Jun 19, 2018

LGTM.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 19, 2018

Codecov Report

Merging #2666 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2666      +/-   ##
==========================================
+ Coverage   62.06%   62.23%   +0.16%     
==========================================
  Files         134      134              
  Lines       21739    21740       +1     
==========================================
+ Hits        13492    13529      +37     
+ Misses       6803     6771      -32     
+ Partials     1444     1440       -4

if service == nil {
// If the service can't be found, assume that it was deleted
// and remove the task from the dirty list.
delete(tr.dirty, dirty)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be moved to cleanup instead of just being deleted from dirty, otherwise they'll never be removed from the store until the next leader election?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Er sorry deleteTasks

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Negative. the dirty map is orchestrator.SlotTuple objects, which represent a service and a node or slot, not an individual task. The task itself should be in the cleanup list already.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That said, the comment ought to be reworded to say remove the slot instead of remove the task

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah ok, thanks!

Copy link
Copy Markdown
Contributor

@cyli cyli left a comment

Choose a reason for hiding this comment

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

LGTM pending comment update.

… service has been deleted.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Jun 19, 2018

LGTM, soon as it passes through CI.

@cyli
Copy link
Copy Markdown
Contributor

cyli commented Jun 19, 2018

Passed, merging.

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