Skip to content
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

Merged
merged 4 commits into from
Jun 1, 2018
Merged

Conversation

aahmed-se
Copy link
Contributor

Add support for Apache Pulsar

@bsideup
Copy link
Member

bsideup commented May 30, 2018

@aahmed-se oh wow, you were faster than me :D 👍

Copy link
Member

@bsideup bsideup left a 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);
Copy link
Member

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

Copy link
Contributor Author

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);
Copy link
Member

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

Copy link
Contributor Author

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);
Copy link
Member

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 💯

Copy link
Contributor Author

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 @@
#
Copy link
Member

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 @@
#
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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 :)

Copy link
Contributor Author

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

Copy link
Member

@bsideup bsideup May 31, 2018

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? :)

Copy link
Contributor Author

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>
Copy link
Member

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

@aahmed-se
Copy link
Contributor Author

comments addressed

@bsideup bsideup merged commit 0fa4118 into testcontainers:master Jun 1, 2018
@bsideup
Copy link
Member

bsideup commented Jun 1, 2018

@aahmed-se merged, thanks! 👍

bsideup added a commit that referenced this pull request Jun 1, 2018
@bsideup bsideup added this to the next milestone Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants