Socat sidecar proxy for Couchbase container to support random ports #815
Conversation
- updated couchbase sdk to support 5.5.0 - added all client ports to port mappings - configuring couchbase through static_config file
- added @ClassRule for the couchbase container tests instead of static variables to prevent race conditions - configured socat to expose the bootstrap ports as target. These bootstrap ports are not added to the couchbase config file.
…ports # Conflicts: # modules/couchbase/src/main/java/org/testcontainers/couchbase/CouchbaseContainer.java
- tool pleasing
| @@ -356,6 +358,10 @@ protected void configure() { | |||
|
|
|||
| } | |||
|
|
|||
| @SuppressWarnings({"EmptyMethod", "UnusedParameters"}) | |||
| protected void containerIsCreated(String containerId) { | |||
There was a problem hiding this comment.
It's actually containerWasCreated, or is it still in process?
There was a problem hiding this comment.
The creation is finished. I just named it in the style of the other callbacks containerIsStarted and containerIsStarting.
There was a problem hiding this comment.
Yes, was assuming something like this and makes sense to keep the style consistent. Just maybe less clear language.
| @@ -556,7 +562,10 @@ public void setWaitStrategy(org.testcontainers.containers.wait.strategy.WaitStra | |||
| * @see #waitingFor(org.testcontainers.containers.wait.strategy.WaitStrategy) | |||
| */ | |||
| protected void waitUntilContainerStarted() { | |||
| getWaitStrategy().waitUntilReady(this); | |||
| org.testcontainers.containers.wait.strategy.WaitStrategy waitStrategy = getWaitStrategy(); | |||
There was a problem hiding this comment.
no, this is the new org.testcontainers.containers.wait.strategy.WaitStrategy which is has a name clash with the deprecated org.testcontainers.containers.wait.WaitStrategy
| @@ -1028,8 +1037,8 @@ public void copyFileToContainer(MountableFile mountableLocalFile, String contain | |||
| @Override | |||
| public void copyFileFromContainer(String containerPath, String destinationPath) throws IOException { | |||
|
|
|||
| if (!isRunning()) { | |||
| throw new IllegalStateException("copyFileToContainer can only be used while the Container is running"); | |||
| if (!isCreated()) { | |||
There was a problem hiding this comment.
Kind of like above, wasCreated() seems more clear.
Would change the exception message accordingly to "after the container was created".
There was a problem hiding this comment.
this is a public (default) method from ContainerState.java which is only used twice in the project. Still I do not feel comfortable to rename it.
There was a problem hiding this comment.
Okay, let's not rename it if we break the public API.
| Stream.<Runnable>of(super::stop, proxy::stop, this::stopCluster).parallel().forEach(Runnable::run); | ||
| } | ||
|
|
||
| private void stopCluster() { |
There was a problem hiding this comment.
what's the point of it? We're forcibly shutting down the container, isn't it enough?
There was a problem hiding this comment.
This shuts down the state of the sdk. If you have multiple tests with different containers, the CouchbaseContainer will create a new environment and the couchbase sdk even warns that you should always reuse the sdk
11:19:46.345 WARN com.couchbase.client.core.env.CoreEnvironment - More than 1 Couchbase Environments found (2), this can have severe impact on performance and stability. Reuse environments!
| private File copyConfigFromContainer(String containerId) throws IOException { | ||
| File tempDirectory = Files.createTempDirectory("testcontainer_" + containerId + "_").toFile(); | ||
| tempDirectory.deleteOnExit(); | ||
| File tempFile = new File(tempDirectory, STATIC_CONFIG_NAME); |
There was a problem hiding this comment.
yes but currently the copy to container functionality does not support renaming the file. Therefore we would copy with the wrong file name. A normal rename of the file is not an option as then it is not really tmp. I would have to change the copy code to support renaming.
|
|
||
| private void appendPortsToConfig(File tempFile) throws IOException { | ||
| for(CouchbasePort port: CouchbasePort.values()) { | ||
| if (! port.isBootstrap()) { |
| } | ||
|
|
||
| private void appendPortsToConfig(File tempFile) throws IOException { | ||
| for(CouchbasePort port: CouchbasePort.values()) { |
|
|
||
| final int originalPort; | ||
|
|
||
| final boolean isBootstrap; |
There was a problem hiding this comment.
I would rename to dynamic, and also mark MEMCACHED as dynamic (as it was in the previous implementation)
There was a problem hiding this comment.
renaming is fine.
Problem with MEMCACHED is, that when marking them as dynamic, the sdk does not work:
11:26:49.578 WARN com.couchbase.client.core.endpoint.Endpoint - [localhost:11210][KeyValueEndpoint]: Could not connect to endpoint on reconnect attempt 5, retrying with delay 32 MILLISECONDS: com.couchbase.client.deps.io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:11210
11:26:49.612 WARN com.couchbase.client.core.endpoint.Endpoint - [localhost:11210][KeyValueEndpoint]: Could not connect to remote socket: Connection refused: localhost/127.0.0.1:11210
It uses the right mapped port for bootstrapping but then discovers the unmapped port from the cluster and then tries to connect to that port and fails with connection refused.
- minor code style and naming improvements
|
We have this out in a Release Candidate build (1.9.0-rc1) for anyone who is keen to try it! |
inspired by #785.