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

Use LogMessageWaitStrategy in VncRecordingContainer #2547

Merged
merged 5 commits into from Apr 12, 2020

Conversation

srempfer
Copy link
Contributor

@srempfer srempfer commented Apr 9, 2020

I run in the same issue as described here #2063
After some investigation I found out that the await timeout is to low in slow environments.
This happens also in my development environment - using Docker for Windows 2.2.0.5.
Unfortunately it tooks more than 1 second to retrieve the logs from the container and so the FrameConsumerResultCallback has no chance to count down the latch before the await timeout appears.
For my environment a await timeout of 2 seconds worked but I thought some more time couldn't hurt because it's only the worst case wait time and normally the logs should be retrieved faster.

@kiview
Copy link
Member

kiview commented Apr 10, 2020

@bsideup @rnorth
Why aren't we using LogMessageWaitStrategy for this container?

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

LGTM, the failing build is a completely unrelated problem with DB2 tests on AZP for now.

@rnorth
Copy link
Member

rnorth commented Apr 10, 2020

🤯 this container predates the existence of LogMessageWaitStrategy... and I think we must have forgotten to retrofit LogMessageWaitStrategy onto it.

We should do that...! I'd be OK with keeping this PR as-is, so that it can at least be released soon.

@bsideup
Copy link
Member

bsideup commented Apr 11, 2020

@kiview @rnorth I agree that it would be better to replace it with the log waiting strategy

@srempfer
Copy link
Contributor Author

srempfer commented Apr 11, 2020

All right, then I will change it to LogMessageWaitStrategy

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

So this will result in 60 seconds default timeout by the way. But thanks for switching out the strategy.

@srempfer
Copy link
Contributor Author

srempfer commented Apr 11, 2020

@bsideup Removed the test
@kiview Reduced the startup timeout to 15 seconds

@rnorth
Copy link
Member

rnorth commented Apr 12, 2020

Merging from master into the PR branch to incorporate #2557 and hopefully fix build.

@bsideup bsideup changed the title Increased latch await timeout to fix start timeout of VncRecordingContainer Use LogMessageWaitStrategy in VncRecordingContainer Apr 12, 2020
@bsideup bsideup merged commit 34b8d7e into testcontainers:master Apr 12, 2020
@rnorth
Copy link
Member

rnorth commented Apr 13, 2020

This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉

Thanks for the contribution!

quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020
…s#2063) (testcontainers#2547)

Co-authored-by: Stefan Rempfer <srempfer@rt.ipnt.de>
Co-authored-by: Richard North <rich.north@gmail.com>
Co-authored-by: Sergei Egorov <bsideup@gmail.com>
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.

None yet

4 participants