Perform the port checks in parallel #4463

bsideup · 2021-09-16T16:43:41Z

No description provided.

rnorth · 2021-09-16T17:00:11Z

core/src/main/java/org/testcontainers/containers/wait/strategy/HostPortWaitStrategy.java

@@ -41,10 +49,52 @@ protected void waitUntilReady() {
        Callable<Boolean> externalCheck = new ExternalPortListeningCheck(waitStrategyTarget, externalLivenessCheckPorts);

        try {
-            Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS,
-                () -> getRateLimiter().getWhenReady(() -> internalCheck.call() && externalCheck.call()));
+            List<Future<Boolean>> futures = EXECUTOR.invokeAll(Arrays.asList(


rnorth · 2021-09-16T17:04:06Z

core/src/main/java/org/testcontainers/containers/wait/strategy/AbstractWaitStrategy.java


 public abstract class AbstractWaitStrategy implements WaitStrategy {

+    static final ExecutorService EXECUTOR = Executors.newCachedThreadPool(new ThreadFactory() {


Could we use Guava's ThreadFactoryBuilder here?

I would rather reduce Guava usage, not increase it 😅 I could change it, but this is some "static" code, and it is not huge, so... WDYT? :)

I'd say not use Guava and use a real class WaitStrategyDaemonThreadFactory or something.

what's wrong with a non-reusable 10 lines anonymous class close to the usage? 😅

That I am confronted with lower-level implementation detail at the head of the file instead of a class name acting as an abstraction. But these are differences in our style, which we might never overcome 😅

I don't mind; I'd rather see the removal of Guava as a dependency too, but it's there.

Perhaps we could extract this to our own class, a bit like @kiview suggests? We have the same code over in Startables.

This has dragged on a bit, so: let’s leave it. A bit of repetition doesn’t do any harm. If we ever add a thread factory again, though, let’s make all occurrences use a shared class

kiview · 2021-09-17T06:13:35Z

Could you measure a performance improvement in certain conditions? For me, it is the same with Kafka.

bsideup · 2021-09-17T09:02:34Z

@kiview this one a bit hard to measure, as it only saves tens or sometimes hundreds of milliseconds, so we have to rely on a common sense of the code (running checks in parallel and cache their results, as opposed to sequential non-cached execution)

That said, I did observe a slight improvement with Kafka (100ms or so)

kiview · 2021-09-17T09:18:09Z

core/src/main/java/org/testcontainers/containers/wait/strategy/HostPortWaitStrategy.java

+                // Polling
+                () -> {
+                    Instant now = Instant.now();
+                    RateLimiter rateLimiter = RateLimiterBuilder


WDYT about replacing duct-tape with awaitility here?

Apparently, Awaitility was performing much slower (~100ms vs 10ms). Need to investigate why (and maybe revisit our previous plans to migrate)

Not sure if directly related to your observation, but the way Awaitility handles its executors by default might introduce some overhead, see awaitility/awaitility#168.

I am an idiot - I missed the fact that, unless explicitly set, pollDelay is equal to pollInterval, so it was adding a static delay 😅 With pollDelay set to 0, Awaitility is as fast as while (...) {} 🎉

Ouh yeah! But this is easy to get wrong, not the most intuitive default IMO.

kiview · 2021-09-17T10:04:54Z

The failing orientdb tests in CI are weird.

rnorth · 2021-09-20T08:54:26Z

Yeah, FTR:

Gradle Test Executor 1 > org.testcontainers.containers.OrientDBContainerTest > shouldReturnTheSameSession FAILED
    com.orientechnologies.orient.core.exception.OSecurityAccessException: Wrong user/password to [connect] to the remote OrientDB Server instance

...

Gradle Test Executor 1 > org.testcontainers.containers.OrientDBContainerTest > shouldInitializeWithCommands FAILED
    com.orientechnologies.orient.core.exception.OSecurityAccessException: Wrong user/password to [connect] to the remote OrientDB Server instance

Will try this multiple times locally; maybe there is a race condition that's become exposed by faster startup.

kiview · 2021-09-20T08:55:45Z

Which would indicate, that the actual WaitStrategy is not stable (which might very well be the case).

rnorth · 2021-09-20T09:42:59Z

Running repeatedly on this branch, I'm getting roughly a 5-10% failure rate on these and other tests in the OrientDB module.

It seems like there's 0% failure rate when running the same tests on master branch.

Maybe this would be a good moment to switch the OrientDB module to use a log-based wait strategy?

Of course I think we (and users) may have to be on the lookout for other race conditions that this faster startup check exposes, which is a real shame 😭.

kiview · 2021-09-20T10:38:51Z

TBH I don't understand how this race condition can happen, given that OrientDBContainer container already uses an additional HttpWaitStrategy:

waitStrategy = new WaitAllStrategy()
            .withStrategy(Wait.forListeningPort())
            .withStrategy(waitForHttp)
            .withStartupTimeout(Duration.ofMinutes(2));

I will little bit look into this, to see what it does at startup.

rnorth · 2021-09-20T10:41:51Z

Perhaps the container is applying configuration while already listening? Have created #4471 as a draft PR to change the strategy a bit.

kiview · 2021-09-20T13:17:58Z

Let's merge #4471 first and then this PR once CI is green. Better to not increase the flakiness of our own CI pipeline (even if the improvement just highlights existing flakiness).

rnorth · 2021-10-12T08:07:21Z

/azp run

azure-pipelines · 2021-10-12T08:07:30Z

Azure Pipelines successfully started running 2 pipeline(s).

This reverts commit 69cf13c.

Perform the port checks in parallel

f5d111c

bsideup added the type/enhancement label Sep 16, 2021

bsideup added this to the next milestone Sep 16, 2021

bsideup requested review from kiview and rnorth as code owners September 16, 2021 16:43

rnorth reviewed Sep 16, 2021

View reviewed changes

kiview reviewed Sep 17, 2021

View reviewed changes

use Awaitility

687eeba

kiview approved these changes Sep 17, 2021

View reviewed changes

Merge branch 'master' into parallel_port_checks

9f7a413

kiview merged commit 69cf13c into master Oct 13, 2021

delete-merged-branch bot deleted the parallel_port_checks branch October 13, 2021 11:12

kiview added a commit that referenced this pull request Oct 21, 2021

Revert "Perform port checks in parallel (#4463)"

289461b

This reverts commit 69cf13c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform the port checks in parallel #4463

Perform the port checks in parallel #4463

bsideup commented Sep 16, 2021

rnorth Sep 16, 2021

rnorth Sep 16, 2021

bsideup Sep 16, 2021

kiview Sep 17, 2021

bsideup Sep 17, 2021

kiview Sep 17, 2021

rnorth Sep 17, 2021

rnorth Oct 12, 2021

kiview commented Sep 17, 2021

bsideup commented Sep 17, 2021 •

edited

Loading

kiview Sep 17, 2021

bsideup Sep 17, 2021

kiview Sep 17, 2021

bsideup Sep 17, 2021

kiview Sep 17, 2021

kiview commented Sep 17, 2021

rnorth commented Sep 20, 2021

kiview commented Sep 20, 2021

rnorth commented Sep 20, 2021

kiview commented Sep 20, 2021

rnorth commented Sep 20, 2021

kiview commented Sep 20, 2021

rnorth commented Oct 12, 2021

azure-pipelines bot commented Oct 12, 2021


		public abstract class AbstractWaitStrategy implements WaitStrategy {

		static final ExecutorService EXECUTOR = Executors.newCachedThreadPool(new ThreadFactory() {

Perform the port checks in parallel #4463

Perform the port checks in parallel #4463

Conversation

bsideup commented Sep 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview commented Sep 17, 2021

bsideup commented Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview commented Sep 17, 2021

rnorth commented Sep 20, 2021

kiview commented Sep 20, 2021

rnorth commented Sep 20, 2021

kiview commented Sep 20, 2021

rnorth commented Sep 20, 2021

kiview commented Sep 20, 2021

rnorth commented Oct 12, 2021

azure-pipelines bot commented Oct 12, 2021

bsideup commented Sep 17, 2021 •

edited

Loading