-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow Pulsar default WaitStrategy to honour startup timeout #5674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am having the same issue with hard override of wait strategy
#5830
084f530
to
5a13395
Compare
waitStrategies.forEach(compoundedWaitStrategy::withStrategy); | ||
waitingFor(compoundedWaitStrategy); | ||
|
||
if (startupTimeout != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great feedback and suggestions. PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
modules/pulsar/src/test/java/org/testcontainers/containers/PulsarContainerTest.java
Show resolved
Hide resolved
waitStrategies.forEach(compoundedWaitStrategy::withStrategy); | ||
waitingFor(compoundedWaitStrategy); | ||
|
||
if (startupTimeout != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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; | |
} |
@Override | ||
public PulsarContainer withStartupTimeout(Duration startupTimeout) { | ||
this.startupTimeout = startupTimeout; | ||
return super.withStartupTimeout(startupTimeout); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can omit this method and already set the WaitAllStrategy
in the constructor. The withStartupTimeout
method of GenericContainer
will work as expected.
1163b73
to
10e06b0
Compare
## 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.
10e06b0
to
0ac293c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for applying all the recommendations @nahguam, was great working with you 👍
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.