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

Retry internal port checking in the same exec #4460

Merged
merged 3 commits into from
Sep 16, 2021

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Sep 15, 2021

Exec'ing with Docker is slow. Even if the command itself takes 10ms to execute, docker exec would take 300ms or so. This PR adds while true-style waiting in the same exec session, so that we pay the overhead of exec only once.

Before:

Container confluentinc/cp-kafka:5.4.3 started in PT5.492S

After:

Container confluentinc/cp-kafka:5.4.3 started in PT4.387S

@bsideup bsideup added this to the next milestone Sep 15, 2021

for (int internalPort : internalPorts) {
command.append(" && ");
command.append(" (");
command.append(format("cat /proc/net/tcp* | awk '{print $2}' | grep -i ':0*%x'", internalPort));
command.append(format("grep -i ':0*%x' /proc/net/tcp*", internalPort));
Copy link
Member Author

Choose a reason for hiding this comment

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

minus dependencies on cat and awk, just in case :)

@kiview
Copy link
Member

kiview commented Sep 16, 2021

LGTM
Only thing I wondered if it makes any sense to still wrap it in Unreliables here:

Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS,
() -> getRateLimiter().getWhenReady(() -> internalCheck.call() && externalCheck.call()));

Wouldn't this mean that a failing external check would still lead to unnecessary exec calls?

At least it does not hurt for now and keeps the PR small.

@bsideup
Copy link
Member Author

bsideup commented Sep 16, 2021

@kiview one additional benefit of still having Unreliables there is to timeout (which will lead to container startup failure and the container being removed, so exec will eventually be terminated)

@kiview
Copy link
Member

kiview commented Sep 16, 2021

Makes total sense.
But maybe splitting () -> internalCheck.call() && externalCheck.call() into a sequence of two distinct Unreliables calls is still beneficial? As per my argument above, no need to re-evaluate internalCheck while waiting for externalCheck to become ready (my assumption).

Let me just try with Kafka, to see if there is an observable difference.

@bsideup
Copy link
Member Author

bsideup commented Sep 16, 2021

@kiview I thought about it (and already have a prepared follow up for caching the results) 👍 Will do right after this one :)

@kiview
Copy link
Member

kiview commented Sep 16, 2021

Cool, just FYI, my naive experiment of

Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS,
    () -> getRateLimiter().getWhenReady(internalCheck));

Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS,
    () -> getRateLimiter().getWhenReady(externalCheck));

made in 1s slower 😱

@rnorth
Copy link
Member

rnorth commented Sep 16, 2021

Oh this is fun; on Mac/Win the external check will possibly pass before the internal check (because of the userland proxy racing the real process). On Linux, if one passes the other really should be a no-op.

This gets weird to reason about if internalCheck is now blocking and externalCheck is polled.

I've deleted my thinking, but wrote more about this. TLDR is that I think we could overthink this, so let's just be guided by the numbers of what works quickly and what's easy to read 😂

@bsideup
Copy link
Member Author

bsideup commented Sep 16, 2021

@rnorth yep, exactly the reason why we perform both :D The hidden gems of Testcontainers :D

@bsideup bsideup merged commit 5e6dbc7 into master Sep 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the retry_internal_port_checking branch September 16, 2021 14:14
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

3 participants