Allow Pulsar default WaitStrategy to honour startup timeout

[nahguam]

Motivation

Currently, the PulsarContainer WaitStrategy is configured after the user
has started the container. This means that any modifications that the
user makes to the WaitStrategy or startupTimeout are discarded.

Modifications

This change honours the user's modifications to the WaitStrategy or
startupTimeout.

[eddumelendez]

thanks for the PR @nahguam ! can you add a test, please? in order to make sure the startupTimeout is honored? Maybe just setting to 0 and expect an exception with the right message will be ok.

[VMarisevs]

I am having the same issue with hard override of wait strategy
#5830

[VMarisevs]

tbh I am not sure if this is a good strategy.
I would rely more on the health end-point 200 response from /admin/v2/namespaces/public

sort of:

 Wait.forHttp("/admin/v2/namespaces/public")
                .forStatusCode(200)
                .forPort(PulsarContainer.BROKER_HTTP_PORT)
                .withStartupTimeout(Duration.ofMinutes(3)

[nahguam]

Given a single startup timeout I think it should be applied to all the sub-strategies as per WaitAllStrategy behaviour:

• mapped ports: 6650, 8080
• admin endpoint
• if enabled, transactions endpoint
• if enabled, functions log entry

I don't think it should just selectively apply to one of them. I think a reasonable alternative would be to expose different startup timeouts for each sub-strategy, but I think that's overkill. Thoughts?

[kiview]

Applying the timeout on the WaitAllStrategy is definitely the way to go here. WaitAllStrategy already provided different timeout strategies, with WITH_OUTER_TIMEOUT being the default.

The implementation of WaitAllStrategy is slightly failsafe, meaning WITH_OUTER_TIMEOUT will work as expected if you call withStartupTimeout after adding all strategies, but also working fine when adding strategies after withStartupTimeout has been applied:

testcontainers-java/core/src/main/java/org/testcontainers/containers/wait/strategy/WaitAllStrategy.java

Lines 70 to 77 in de1324e

	public WaitAllStrategy withStrategy(WaitStrategy strategy) {
	if (mode == Mode.WITH_OUTER_TIMEOUT) {
	applyStartupTimeout(strategy);
	}
	this.strategies.add(strategy);
	return this;
	}

[nahguam]

Thanks for the great feedback and suggestions. PTAL

[kiview]

Thanks a lot for the PR @nahguam. I think if we change the implementation to set the WaitAllStrategy in the constructor, it will cleanup the code (we don't need a custom withStartupTimeout implementation) and make the container behave in general more as expected with regards to custom configuration by the user.

[kiview]

Applying the timeout on the WaitAllStrategy is definitely the way to go here. WaitAllStrategy already provided different timeout strategies, with WITH_OUTER_TIMEOUT being the default.

The implementation of WaitAllStrategy is slightly failsafe, meaning WITH_OUTER_TIMEOUT will work as expected if you call withStartupTimeout after adding all strategies, but also working fine when adding strategies after withStartupTimeout has been applied:

testcontainers-java/core/src/main/java/org/testcontainers/containers/wait/strategy/WaitAllStrategy.java

Lines 70 to 77 in de1324e

	public WaitAllStrategy withStrategy(WaitStrategy strategy) {
	if (mode == Mode.WITH_OUTER_TIMEOUT) {
	applyStartupTimeout(strategy);
	}
	this.strategies.add(strategy);
	return this;
	}

[kiview]

We can omit this method and already set the WaitAllStrategy in the constructor. The withStartupTimeout method of GenericContainer will work as expected.

[kiview]

Thanks for applying all the recommendations @nahguam, was great working with you 👍