-
-
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
Update Pulsar Container #858
Conversation
Hi @aahmed-se! It seems that 2.2.0 is not released yet :D |
my mistake it's actually another change for something else, will mark it wip for now |
b714d28
to
4b1dc1c
Compare
This is ready for review now |
4b1dc1c
to
c97fa9f
Compare
|
||
public PulsarContainer() { | ||
this("2.1.0-incubating"); | ||
super(IMAGE_NAME); |
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.
could you please revert constructor removal? That's a breaking change
waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1)); | ||
@Override | ||
public void start() { | ||
this.waitStrategy = new HttpWaitStrategy() |
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.
please use waitingFor
during the configuration
*/ | ||
public class PulsarContainer extends GenericContainer<PulsarContainer> { | ||
public class PulsarContainer<T extends PulsarContainer<T>> extends GenericContainer<T> { |
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.
this container is a final container, there is no need for the self-typing
c97fa9f
to
45e6860
Compare
Comments addressed |
waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1)); | ||
withExposedPorts(BROKER_HTTP_PORT, BROKER_PORT); | ||
withCommand("/bin/bash", "-c", "/pulsar/bin/pulsar standalone --no-functions-worker -nss"); | ||
waitingFor(Wait.forHttp(METRICS_ENDPOINT).forStatusCode(200)); |
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.
would be better to specify port here explicitly, otherwise it uses "first exposed port" and users might change exposed ports according to their needs
|
||
waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1)); | ||
withExposedPorts(BROKER_HTTP_PORT, BROKER_PORT); | ||
withCommand("/bin/bash", "-c", "/pulsar/bin/pulsar standalone --no-functions-worker -nss"); |
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.
fyi should also work with withCommand("/pulsar/bin/pulsar standalone", "--no-functions-worker", "-nss");
45e6860
to
47243e1
Compare
Made the changes |
@bsideup let me know if there any more comments. |
@bsideup is there anything else to change ? |
@aahmed-se it looks great, thank you 👍 |
Released for preview in 1.9.0-rc2, to be published on Bintray. Thanks @aahmed-se! |
Simply startup script using defaults.