Skip to content

Parallelize TestEventsLimit#12719

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
fntlnz:12675-test-events-emit
Apr 27, 2015
Merged

Parallelize TestEventsLimit#12719
cpuguy83 merged 1 commit intomoby:masterfrom
fntlnz:12675-test-events-emit

Conversation

@fntlnz
Copy link
Copy Markdown
Contributor

@fntlnz fntlnz commented Apr 23, 2015

fix #12675
Signed-off-by: Lorenzo Fontana fontanalorenzo@me.com

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.

I'm afraid there's a race there: we're starting 30 goroutines but immediately call for docker events, I don't think we have any guarantee to reliability get the 64 expected events in the end.

I'd add a sync.WaitGroup: @LK4D4 what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also can't use dockerCmd in a goroutine like that since it calls c.Fatal on error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Better to use directly runCommandWithOutput ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, and any error would need to be dealt with from the main goroutine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Better synchronize everything with a channel to handle eventual errors from dockerCmd runCommandWithOutput and I'd use another to handle docker events streaming checks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your hints :)

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 be really buferized to 30, to let goroutines just die.

@fntlnz
Copy link
Copy Markdown
Contributor Author

fntlnz commented Apr 26, 2015

@LK4D4 looks good to you?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit, I'd call it errChan since errors could be imported (again, nit, don't necessary do it and maintainers have last word of course, wait for them and in case make this change)

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.

Yeah, errors is package from stdlib, so, can be confusing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thank you, I'm fixing this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, all done!

Signed-off-by: Lorenzo Fontana <fontanalorenzo@me.com>
@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Apr 27, 2015

For me test 4 times faster.
LGTM

@cpuguy83
Copy link
Copy Markdown
Member

LGTM

cpuguy83 added a commit that referenced this pull request Apr 27, 2015
@cpuguy83 cpuguy83 merged commit 2f513db into moby:master Apr 27, 2015
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.

TestEventsLimit is too long

7 participants