-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add Apache Pulsar Support #713
Conversation
@aahmed-se oh wow, you were faster than me :D 👍 |
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 love it! Just requested a few minor changes, but looks awesome in general, I thought it will be much harder 👍
withNetwork(Network.newNetwork()); | ||
String networkAlias = "pulsar-" + Base58.randomString(6); | ||
withNetworkAliases(networkAlias); | ||
withExposedPorts(PULSAR_PORT); |
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.
network is not needed here (was only needed for Kafka), only withExposedPorts
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.
done
|
||
@Override | ||
public void start() { | ||
withClasspathResourceMapping("proxy.conf", "/pulsar/conf/proxy.conf", BindMode.READ_ONLY); |
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 move these to the constructor
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.
done
|
||
@Override | ||
public void stop() { | ||
Stream.<Runnable>of(super::stop).parallel().forEach(Runnable::run); |
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 override is not needed.
In Kafka, we have to start a few more containers besides Kafka, but Pulsar is much better and does not need that 💯
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.
done
@@ -0,0 +1,104 @@ | |||
# |
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 file seems to be committed by accident ( modules/pulsar/out/production/
folder)
@@ -0,0 +1,104 @@ | |||
# |
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.
will it work without this file? If so, it would be better to avoid file mounting because in some rare environments that's not possible
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 do need it simply because we need to change the default port of the embedded proxy , otherwise we will have to manipulate the advertised address which I prefer less.
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.
Do you think it's possible to keep only changed properties? Currently it's a bit unclear what was changed compared to the default config :)
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 don't think so the best I can think of is to add comments
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 was able to do it like this:
withCommand("/bin/bash", "-c", "" +
"servicePort=6850 webServicePort=8280 webServicePortTls=8643 bin/apply-config-from-env.py conf/proxy.conf && " +
"bin/pulsar standalone & " +
"bin/pulsar proxy --zookeeper-servers localhost:2181 --global-zookeeper-servers localhost:2181"
);
waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1));
WDYT? :)
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.
that's fine will go with that.
@@ -0,0 +1,18 @@ | |||
<configuration> |
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.
the same as my previous comment about modules/pulsar/out/
folder
comments addressed |
@aahmed-se merged, thanks! 👍 |
Add support for Apache Pulsar