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

Support parallel kill,rm #26778

Merged
merged 2 commits into from
Oct 10, 2016
Merged

Conversation

WeiZhang555
Copy link
Contributor

Signed-off-by: Zhang Wei zhangwei555@huawei.com

@WeiZhang555 WeiZhang555 changed the title Support parallel kill Support parallel kill,rm Sep 21, 2016
}

errChan := parallelOperation(ctx, opts.containers, func(ctx context.Context, id string) error {
if id == "" {
return fmt.Errorf("Container name cannot be empty")
Copy link
Member

Choose a reason for hiding this comment

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

"Container ID .."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not indeed. It can be either container id or container name, up to input from user. 😄

@WeiZhang555
Copy link
Contributor Author

ping @thaJeztah @vdemeester @mlaventure

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

One nit comment but overall LGTM 🐸

}

errChan := parallelOperation(ctx, opts.containers, func(ctx context.Context, id string) error {
if id == "" {
Copy link
Member

Choose a reason for hiding this comment

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

nameOrId 👼 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe container is better? it covers name and id 😄

Copy link
Member

Choose a reason for hiding this comment

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

true 👍

Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
@duglin
Copy link
Contributor

duglin commented Sep 28, 2016

I'm a bit concerned about this PR, but I might be missing something in the code. How does this impact the UX of the cmds? For example, how do we guarantee that the output isn't overlapping? Right now with cmds that take multiple containers as args there's a guarantee that the output will always be in a certain order, and it will follow the exact list of container IDs specified on the cmd line. This allows people to easily correlate which bit of output goes with which container. This PR appears to remove that guarantee, meaning the user then has to figure out which output goes with which container. Do we guarantee that ALL output from each kill/rm has enough uniquely identifying information (like the container name/id) in the output to always allow this correlation?

@WeiZhang555
Copy link
Contributor Author

@duglin don't worry, we have the output order gurantee 😄 . See #24761 (comment) in PR: #24761

I made a generic public function parallelOperation, which will execute some function and return result in same order with input.

@duglin
Copy link
Contributor

duglin commented Sep 28, 2016

ok cool - wasn't sure from looking at the code. I did notice we show the errors/ID in the right order, but wasn't sure if there would ever be any other output that's sent to stderr/stdout. Guess not.

@WeiZhang555
Copy link
Contributor Author

@duglin I think the output should be right. What do you think of this? 😄

@WeiZhang555
Copy link
Contributor Author

What's the current status? ping @mlaventure @thaJeztah

@thaJeztah thaJeztah added this to the 1.13.0 milestone Oct 10, 2016
@mlaventure
Copy link
Contributor

This LGTM 👍

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Tested this, and works as advertised

LGTM as well

@thaJeztah
Copy link
Member

Don't think we need documentation changes for this, other than a mention in the changelog

@thaJeztah thaJeztah merged commit 2f12d28 into moby:master Oct 10, 2016
@WeiZhang555 WeiZhang555 deleted the parallel-operations branch October 11, 2016 01:54
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants